Skip to content
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.

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

[naming-convention] Allow quoted object keys in any format #1483

Closed
sindresorhus opened this issue Jan 21, 2020 · 12 comments · Fixed by #2071 or #2813
Closed

[naming-convention] Allow quoted object keys in any format #1483

sindresorhus opened this issue Jan 21, 2020 · 12 comments · Fixed by #2071 or #2813
Labels
documentation Documentation ("docs") that needs adding/updating package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@sindresorhus
Copy link

{
	rules: {
		'@typescript-eslint/naming-convention': [
			'error',
			{
				selector: 'default',
				format: [
					'strictCamelCase'
				],
				leadingUnderscore: 'allow',
				trailingUnderscore: 'allow'
			}
		}
	}
}
response.writeHead(413, {
	'Retry-After': retryAfterOn413
});

Expected Result

I expected the rule to ignore quoted object keys or have a way to ignore them. I think the old camelcase rule ignored such case.

Actual Result

✖   16:3   Property name Retry-After must match one of the following formats: strictCamelCase                      @typescript-eslint/naming-convention

Additional Info

Versions

package version
@typescript-eslint/eslint-plugin 2.17.0
@typescript-eslint/parser 2.17.0
TypeScript 3.7.5
ESLint 6.8.0
node 10.17.0
npm 6.13.6
@sindresorhus sindresorhus added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels Jan 21, 2020
@bradzacher
Copy link
Member

bradzacher commented Jan 21, 2020

The base camelcase rule only matched identifiers, yes - so it skipped literals.

I don't think I agree with this entirely, as literal prop names could then slip through, accidentally circumventing the rule.

/*
eslint '@typescript-eslint/naming-convention': [
			'error',
			{
				selector: 'default',
				format: [
					'strictCamelCase'
				],
				leadingUnderscore: 'allow',
				trailingUnderscore: 'allow'
			}
		}
*/
const x = {
  'Skip This': 1, // sure, it seems obvious that this should be skipped
  x_x: 1, // error - not in strictCamelCase
  'y_y': 1, // this should error too, right?
}

A surprising number of people don't use prettier, and the base quote-props rule isn't recommended, so it's easy for people to accidentally quote props.

I don't know the best solution here. Part of me thinks maybe this is just an eslint-disable case. Unsure, need to think some more. Maybe it should inspect the name to see if it requires quotes, and ignores it if it does?

@bradzacher bradzacher added bug Something isn't working enhancement New feature or request and removed bug Something isn't working labels Jan 21, 2020
@sindresorhus
Copy link
Author

as literal prop names could then slip through, accidentally circumventing the rule.

I would argue that's an explicit intent on using something else.

A surprising number of people don't use prettier, and the base quote-props rule isn't recommended, so it's easy for people to accidentally quote props.

You could make it an option to ignore quote object literal keys or expose the type so I can manually ignore it myself.

Part of me thinks maybe this is just an eslint-disable case.

It's quite a common use-case though. You would then force pretty much any server-related app to use lots of eslint-disable comments as you kinda have to do this when dealing with HTTP headers.

Maybe it should inspect the name to see if it requires quotes, and ignores it if it does?

That would be the preferred solution, yes.

@bradzacher bradzacher added enhancement: plugin rule option New rule option for an existing eslint-plugin rule and removed triage Waiting for maintainers to take a look enhancement New feature or request labels Jan 21, 2020
@thynson

This comment has been minimized.

@danielsanfr
Copy link

Hello people. Thank you for the excellent work with this tool.

I'm having the same problem. Is there any way to resolve it other than to disable it as a comment?

@thynson

This comment has been minimized.

@bradzacher

This comment has been minimized.

@bradzacher
Copy link
Member

bradzacher commented May 22, 2020

Maybe it should inspect the name to see if it requires quotes, and ignores it if it does?

Digging into this - it's actually really, really, really hard to tell if a string is a valid identifier. The grammar for an identifier is very permissive.

The only way to do this reliably would be to add an external library, and I don't hugely want to do that.

Prettier uses esutils to do this:

https://github.com/prettier/prettier/blob/82676f6308322496d71d8ea43853190e710e678f/src/language-js/utils.js#L728-L741

And you can see the regexes esutils auto-generates to do this check here:
https://github.com/estools/esutils/blob/a825f91fd1d1e3a9fff84227cb06c011d8a0b9e8/lib/code.js#L31-L37

However, I never thought about this - but this is entirely achievable from a configuration standpoint.

You can use the filter option to ignore specific names only:

{
  "@typescript-eslint/naming-convention": [
    "error",
    {
      "selector": "property",
      "format": ["strictCamelCase"],
      "filter": {
        // you can expand this regex to add more allowed names
        "regex": "^(Property-Name-One|Property-Name-Two)$",
        "match": false
      }
    }
  ]
}

You can use the filter option to ignore names that require quoting:

{
  "@typescript-eslint/naming-convention": [
    "error",
    {
      "selector": "property",
      "format": ["strictCamelCase"],
      "filter": {
        // you can expand this regex as you find more cases that require quoting that you want to allow
        "regex": "[-]",
        "match": false
      }
    }
  ]
}

I have updated the documentation appropriately.

@bradzacher bradzacher added documentation Documentation ("docs") that needs adding/updating and removed enhancement: plugin rule option New rule option for an existing eslint-plugin rule labels May 22, 2020
@danielsanfr
Copy link

danielsanfr commented May 23, 2020

Hello, thank you very much for responding to our problem.

Sorry if I didn't understand the way you solved the problem, but for my case I had to do it a little differently. I had the following case:

const headers = {
    'Retry-After': retryAfter,
    'X-RateLimit-Limit': limit,
    'X-RateLimit-Remaining': remaining,
    'X-RateLimit-Reset': resete
}

And to resolve, based on your help I did so:

{
    "selector": "property",
    "format": ["PascalCase"],
    "filter": {
        "regex": "[-]",
        "match": true
    }
}

@sindresorhus
Copy link
Author

The only way to do this reliably would be to add an external library, and I don't hugely want to do that.

Instead, you're putting the burden on every user.

Couldn't ESLint potentially expose an utility for this?

@bradzacher
Copy link
Member

burden on every user

only users that set headers via string keys. There aren't many other common use cases that I know of. I believe that this is a small fraction of users.

Even then using a configuration option as above is better as you can thoroughly customise the allowed formats and names based on your use case.

For example:

  • one codebase might specify a custom format that only allows string keys in the form Foo-Bar
  • another might specifically whitelist what string keys are allowed,
  • and a final one might just allow any string key with a - in it.

The configuration is flexible so you can apply the level of strictness you want (which is the entire point of merging all the naming rules together).

Having an ignoreStringKeys option is far reaching, and means you could (esp without prettier) have names like 'foo_bar' or 'Foo Bar' in when you only wanted to allow 'Foo-Bar'.

Couldn't ESLint potentially expose an utility for this?

ESLint exposes no utilities to help with writing rules.
eslint-utils is the closest analogy, and that is technically 3rd-party.

@sindresorhus
Copy link
Author

I believe that this is a small fraction of users.

Neither of us have any proof on that. It's common when doing HTTP requests.

The configuration is flexible so you can apply the level of strictness you want (which is the entire point of merging all the naming rules together).

Flexibility is great, but I also feel that defaults should be good.

Having an ignoreStringKeys option is far reaching

Yes, I wouldn't want such an option. Agreed.

ESLint exposes no utilities to help with writing rules.

It should though, but I realize this is not the place to discuss that.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 24, 2020
bradzacher added a commit that referenced this issue Nov 25, 2020
Fixes #2761
Fixes #1483

This modifier simply matches any member name that requires quotes.
To clarify: this does not match names that have quotes - only names that ***require*** quotes.
bradzacher added a commit that referenced this issue Nov 25, 2020
Fixes #2761
Fixes #1483

This modifier simply matches any member name that requires quotes.
To clarify: this does not match names that have quotes - only names that ***require*** quotes.
@bradzacher
Copy link
Member

Just notifying anyone subscribed to this issue - #2813 adds the requiresQuotes modifier for member-like things.
This modifier will specifically match names that require quotes.

If you simply want to allow all property names that require quotes, you can use the requiresQuotes modifier to match any property name that requires quoting, and use format: null to ignore the name.

New example from the docs for ignoring property names that require quotes (you can customise this to limit it if you see fit).

{
  "@typescript-eslint/naming-convention": [
    "error",
    {
      "selector": [
        "classProperty",
        "objectLiteralProperty",
        "typeProperty",
        "classMethod",
        "objectLiteralMethod",
        "typeMethod",
        "accessor",
        "enumMember"
      ],
      "format": null,
      "modifiers": ["requiresQuotes"]
    }
  ]
}

bradzacher added a commit that referenced this issue Nov 25, 2020
#2813)

Fixes #2761
Fixes #1483

This modifier simply matches any member name that requires quotes.
To clarify: this does not match names that have quotes - only names that ***require*** quotes.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Documentation ("docs") that needs adding/updating package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
4 participants