Skip to content

[naming-convention] camelCase allows kebab-case in property #4534

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
3 tasks done
koooge opened this issue Feb 9, 2022 · 3 comments · Fixed by #4582
Closed
3 tasks done

[naming-convention] camelCase allows kebab-case in property #4534

koooge opened this issue Feb 9, 2022 · 3 comments · Fixed by #4582
Labels
accepting prs Go ahead, send a pull request that resolves this issue bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@koooge
Copy link
Contributor

koooge commented Feb 9, 2022

Hi there,

  • I have tried restarting my IDE and the issue persists.
  • I have updated to the latest version of the packages.
  • I have read the FAQ and my problem is not listed.

Repro

{
  "rules": {
    camelcase: 'off',
    '@typescript-eslint/naming-convention': [
      'error',
      {
        selector: 'default',
        format: ['camelCase'],
        leadingUnderscore: 'allow',
        trailingUnderscore: 'allow',
      },
      {
        selector: 'variable',
        format: ['camelCase', 'UPPER_CASE'],
        leadingUnderscore: 'allow',
        trailingUnderscore: 'allow',
      },
      {
        selector: 'typeLike',
        format: ['PascalCase'],
      },
    ],
  },
};
type Foo = {
  'foo-bar': string;
};

interface Bar {
  'foo-bar': string;
}

Expected Result
It is supposed to be error:

1: 3 error Type property name 'foo-bar' must match one of the following formats: camelCase @typescript-eslint/naming-convention

Actual Result
No error.

Additional Info

Versions

package version
@typescript-eslint/eslint-plugin 5.10.0
@typescript-eslint/parser 5.10.0
TypeScript 4.5.4
ESLint 8.7.0
node 16.13.2
@koooge koooge added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Feb 9, 2022
@bradzacher
Copy link
Member

bradzacher commented Feb 9, 2022 •
edited
Loading

I was going to say that this was intentional, but then I noticed the examples I'd placed in the readme work via sheer dumb luck!
The examples all use strictCamelCase, and then have strings that start with an upper case letter - so they violate the format not because of spaces or dashes, but because of that first letter 🤦 .

The current checkers for pre-defined formats by design assume the name is a "valid identifier" (i.e. no spaces or dashes).

typescript-eslint/packages/eslint-plugin/src/rules/naming-convention-utils/format.ts

Lines 17 to 41 in 2794631

function isPascalCase(name: string): boolean {
return (
name.length === 0 ||
(name[0] === name[0].toUpperCase() && !name.includes('_'))
);
}
function isStrictPascalCase(name: string): boolean {
return (
name.length === 0 ||
(name[0] === name[0].toUpperCase() && hasStrictCamelHumps(name, true))
);
}
function isCamelCase(name: string): boolean {
return (
name.length === 0 ||
(name[0] === name[0].toLowerCase() && !name.includes('_'))
);
}
function isStrictCamelCase(name: string): boolean {
return (
name.length === 0 ||
(name[0] === name[0].toLowerCase() && hasStrictCamelHumps(name, false))
);
}

We should probably make each one also enforce that the string is also a valid identifier using this util:
https://github.com/typescript-eslint/typescript-eslint/blob/27946318b0b4ad7fb2d17428b4b335e6827032d1/packages/type-utils/src/requiresQuoting.ts

Perhaps we can just pass the pre-evaluated modifiers for the name into the format function, and check for the .requiresQuoting modifier (that way we don't to the check twice)

@bradzacher bradzacher added accepting prs Go ahead, send a pull request that resolves this issue bug Something isn't working and removed triage Waiting for team members to take a look labels Feb 9, 2022
@lonyele
Copy link
Contributor

lonyele commented Feb 13, 2022 •
edited
Loading

@bradzacher I was looking at the code and found the fix looks simple which makes me think that I didn't fully understand your approach.
If I add this requiresQuoting check to this place

typescript-eslint/packages/eslint-plugin/src/rules/naming-convention-utils/validator.ts

Lines 385 to 390 in 5b7d8df

for (const format of formats) {
const checker = PredefinedFormatToCheckFunction[format];
if (checker(name)) {
return true;
}
}

like this.

for (const format of formats) {
  const checker = PredefinedFormatToCheckFunction[format];
  if (!util.requiresQuoting(name) && checker(name)) {
    return true;
  }
}

I can pass other tests and get this issue's expected result
'Type Property name 'foo Bar' must match one of the following formats: camelCase' or
'Type Property name 'boo-----foo' must match one of the following formats: camelCase'

I could have guarded using requiresQuoting inside of all 6 predefined formats functions but I chose this one because it looks simpler when no other places are using these predefined formats

or

typescript-eslint/packages/eslint-plugin/src/rules/naming-convention-utils/validator.ts

Lines 374 to 383 in 5b7d8df

function validatePredefinedFormat(
config: NormalizedSelector,
name: string,
node: TSESTree.Identifier | TSESTree.PrivateIdentifier | TSESTree.Literal,
originalName: string,
): boolean {
const formats = config.format;
if (formats === null || formats.length === 0) {
return true;
}

typescript-eslint/packages/eslint-plugin/src/rules/naming-convention-utils/validator.ts

Line 130 in 5b7d8df

if (!validatePredefinedFormat(config, name, node, originalName)) {

somewhere at the front that it doesn't need to be iterated at all ?

do you think this is the viable fix? if it is, then I'd make a PR for it

@bradzacher
Copy link
Member

Nope - you're right! It should be super simple to do!
Ideally we wouldn't call util.requiresQuoting on every format because it does some really complicated regex checks

Given that where it counts we already add a modifier to the name which specifies whether or not it requires quotes:

typescript-eslint/packages/eslint-plugin/src/rules/naming-convention.ts

Lines 114 to 116 in 877cc48

if (requiresQuoting(key, compilerOptions.target)) {
modifiers.add(Modifiers.requiresQuotes);
}

typescript-eslint/packages/eslint-plugin/src/rules/naming-convention.ts

Lines 423 to 425 in 877cc48

if (requiresQuoting(id, compilerOptions.target)) {
modifiers.add(Modifiers.requiresQuotes);
}

We know all of our pre-defined formats are valid identifier string (so don't require quoting). Which means all we should need to do is pass the modifiers into validatePredefinedFormat and then check like if (modifiers.has(Modifiers.requiresQuotes)) { return false }.

It should be really simple!

@lonyele lonyele mentioned this issue Feb 22, 2022
Merged
3 tasks
This was referenced Apr 20, 2022
Merged
Merged
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
accepting prs Go ahead, send a pull request that resolves this issue bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(eslint-plugin): [naming-convention] cover case that requires quotes lonyele/typescript-eslint
3 participants
@koooge @bradzacher @lonyele