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

feat(eslint-plugin): add rule naming-conventions #1318

Merged
merged 36 commits into from Jan 13, 2020
Merged

Conversation

@bradzacher
Copy link
Member

bradzacher commented Dec 9, 2019

Fixes #515
Fixes #590
Fixes #671
Fixes #722
Fixes #871
Fixes #1145

This is a simplified implementation of the naming-conventions rule from tslint-consistent-codestyle.

Unifying all of the naming rules into once place, and adding a lot of options.

Configuration will look something like:

{
"@typescript-eslint/naming-conventions": ["error",
  { selector: "default", format: ["camelCase"] },

  { selector: "variableLike", format: ["camelCase"] },
  { selector: "variable", format: ["camelCase", "UPPER_CASE"] },
  { selector: "parameter", format: ["camelCase"], leadingUnderscore: "allow" },

  { selector: "memberLike", format: ["camelCase"] },
  { selector: "memberLike", modifiers: ["private"], format: ["camelCase"], leadingUnderscore: "require"  },

  { selector: "typeLike", format: ["PascalCase"] },
  { selector: "typeParameter", format: ["PascalCase"], prefix: ["T"] },

  { selector: "interface", format: ["PascalCase"], custom: { regex: "^I[A-Z]", match: false } },
],
}
@glen-84

This comment has been minimized.

Copy link

glen-84 commented Dec 10, 2019

Thanks for working on this! 🙂

  1. Which parts are simplified? I see that some modifiers are not supported, like global, const, export, and unused (which would be useful). Are there other differences?

    My configuration would probably be quite similar to the one here, except:

    • strictCamelCase instead of camelCase.
    • No leading underscores on private/protected properties.
    • Not sure about the toJSON exception ... see question below about external code.
    • No I prefix on interfaces.
    • genericTypeParameter might support some other formats.
  2. Is "external" code checked (code imported from non-relative paths, etc.), and if so, is there a way to ignore that?

    • For example, if I have import {GraphQLResolveInfo} from "graphql";, can I ignore the non-strict naming of GraphQLResolveInfo because it's not my code?
  3. Is your example outdated? Not sure what the variableLike/memberLike selectors are, I don't see them in the source.

  4. Will you be deprecating these rules?

    @typescript-eslint/camelcase
    @typescript-eslint/class-name-casing
    @typescript-eslint/generic-type-naming
    @typescript-eslint/interface-name-prefix
    @typescript-eslint/member-naming

@bradzacher

This comment has been minimized.

Copy link
Member Author

bradzacher commented Dec 10, 2019

Is your example outdated? Not sure what the variableLike/memberLike selectors are, I don't see them in the source.

Just pushed it. I didn't think anyone was looking just yet, so I had some local commits.


Which parts are simplified? I see that some modifiers are not supported, like global, const, export, and unused (which would be useful). Are there other differences?

I got rid of some modifiers. I didn't hugely see the point of many of them. I figure it's better to implement a useful subset rather than go for 1:1 implementation, when it's already a really complex rule.

  • unused - IMO you shouldn't have unused variables etc in your actual code, so I saw little point. Plus supporting this involves adding a lot of code to track usages, so I don't think it's worth adding. (esp when so many people use the no-unused-vars rule).
  • global/local/export involves doing some parent context analysis. I again didn't see the point in adding a bunch of code to traverse up and detect these. I've never seen naming schemes which actually use these. Could always add them later though.
  • rename just seemed useless to me - why someone would want to enforce a different name for const x = ... vs const { prop: x } = ... is beyond me. Again didn't think it was worth doing the parent context analysis.
  • const for variables I didn't bother with purely because I didn't think it was hugely useful - esp when many users use prefer-const, which has a fixer, and would cause no end of naming annoyance when the auto fixer changes let to const.

I removed a selectors:

  • functionVariable, because it will be covered by { selector: 'variable', type: 'function' }.

Also I removed the deep nesting and extension based design of the selectors (i.e. the original had parameterProperty which extended property which extended member which extended default).
I didn't hugely understand it - it wasn't clear to me how it worked exactly and what the "extending" was doing, and what the difference between a final selector and a normal one was.
Ultimately I decided that this is linting config, which means it is going to be configured once. Having all that complex magic seemed silly when you can just have a user be verbose but explicit.

I still included some grouping selectors (variableLike, memberLike, and typeLike), but they are just a simple shortcut (variableLike is the same as writing the selector three times, once for variable, function and parameter).

Finally, I have removed the complete power of defining a custom format regex (i.e. you must use a predefined format) for this first cut. I think this feature needs a lot of thought put into it because all of the predefined formats are actually really hard to express in regex. We can revisit this later.


My configuration would probably be quite similar to the one here, except:
...
No I prefix on interfaces.

I was thinking about this - I think the original implementation was missing a way to ban prefixes (right now it only lets you assert there is one of a set). Thinking about adding this in (so I can completely deprecate interface-name-prefix).

Not sure about the toJSON exception

The toJSON exception is because toJSON is a special method which is called on objects when traversed by JSON.stringify (see this). It's not about external code.

Is "external" code checked (code imported from non-relative paths, etc.), and if so, is there a way to ignore that?

Right now this rule won't actually check import statements at all. Looking at the original code, I don't think it did either.


Will you be deprecating these rules?

Included this in the last push.

@typescript-eslint/camelcase
@typescript-eslint/class-name-casing
@typescript-eslint/generic-type-naming
@typescript-eslint/member-naming

yes times 4

@typescript-eslint/interface-name-prefix

Yes and no...

If the rule goes out with the current config for only checking for the existence of prefixes (not being able to ban them), then I would want to keep this rule, but cut it right back to only be an unconfigurable rule that does "ban I prefix" on interfaces.

If I enhance it to allow banning, then yes, will be deprecated.

@glen-84

This comment has been minimized.

Copy link

glen-84 commented Dec 10, 2019

  • unused – I use noUnusedParameters, so you have to use an underscore to suppress TypeScript's error. For example:

    onComplete: (_id, name, responseJson) => {
        // Using name and responseJson, but not id.
    }

    See also: ajafff/tslint-consistent-codestyle#9

  • global/local/export – useful for enforcing UPPER_CASE in certain cases (f.e. exported constants).

  • rename – I don't really have a use for this. I guess it might be useful if you're consuming an API that uses snake_case but want to enforce camelCase when destructuring from API response objects (const { first_name: firstName } = user;).

  • const – as mentioned above, might be useful for global/exported constants.

I removed a selectors ...

Makes sense to me.

Also I removed the deep nesting and extension based design of the selectors ...

I liked the look of this feature, but perhaps in practice it's not all that important. I'll have to see what my configuration looks like when this rule is released.

I understood it like this: default > member > property > parameterProperty

parameterProperty inherits its configuration from property, which inherits its configuration from member, which inherits its configuration from default.

i.e. If you don't override settings for parameterProperty, it gets the same settings as property.

And final meant that a configuration should not affect "subtypes", for example if you define a rule for property, making it final would mean that "parameterProperty should not inherit these settings".

But ya, I can imagine it being quite tricky to implement.

Finally, I have removed the complete power of defining a custom format regex ...

It's difficult to predict everyone's use cases. I know that camelCase checking is quite difficult with regular expressions, but there are other things that it could be useful for.

At the moment I use a regex like ^[A-Z]([A-Z][a-zA-Z]+|\\d+)?$ for generic-type-naming, to support naming like:

T, U, V (or any single capital letter)
TKey, TValue (single capital letter followed by capitalized word)
T1, T20 (single capital letter followed by one or more numbers)

Regular expressions may not be able to solve all problems, but they are very flexible.

I was thinking about this ...

I guess it was possible with regular expressions ... something like ^(?!I[A-Z]).

The toJSON exception is because toJSON is a special method which is called on objects ...

Ah right, got it. He's not using strict camelCase though, so this probably wasn't even necessary. I'll probably need it though.

Right now this rule won't actually check import statements at all.

Oh, okay. Cool.

Yes and no...

Makes sense, I agree.

@glen-84

This comment has been minimized.

Copy link

glen-84 commented Dec 10, 2019

Oh, I wanted to ask about #816 – would it be easy to add support for this?

If leadingUnderscore was set to, for example, allowBackingField, then it would check if there was a getter or setter within the same class with the same name (less the underscore), and if so, would allow it to have the leading underscore.

@glen-84

This comment has been minimized.

Copy link

glen-84 commented Dec 10, 2019

Should also close:

Could also close (with some extra work):

  • #143 (for filter, at least)
@bradzacher

This comment has been minimized.

Copy link
Member Author

bradzacher commented Dec 10, 2019

Could also close (with some extra work):
#143 (for filter, at least)

Unfortunately not, because JSON Schema doesn't allow for RegExp objects.
This is just an unfortunate limitation of JSON Schemas.


Oh, I wanted to ask about #816 – would it be easy to add support for this?

I think for the first cut, probably not.
But the sky is the limit with this. It'll be a bit difficult, but it's certainly possible.
I'd guess there'd be some kind of modifier (backingField?) for it which is only added if the property shares a name with a getter/setter and it is used within that function body.


I guess it was possible with regular expressions ... something like ^(?!I[A-Z]).

Yeah I want to avoid regexes where possible. Usually it's better to be explicit.
The prefix/suffix option only allows exact strings. I was thinking i'd change it to be an object i.e., prefix/suffix: Array<{ affix: string, opt: 'allow' | 'forbid' | 'require' }>.


At the moment I use a regex like ^[A-Z]([A-Z][a-zA-Z]+|\\d+)?$ for generic-type-naming, to support naming like...

I already documented the affix trimming, but I didn't explicitly state in the docs that an empty string matches all formats. You can get an empty string name if you trim all underscores/affixes.

I.e. { selector: 'typeParameter', format: ['PascalCase'], prefix: ['T', 'U'] } will probably handle the majority of the cases for generic naming. It should allow all of your examples. I'll be sure to add explicit tests for it.


unused – I use noUnusedParameters, so you have to use an underscore to suppress TypeScript's error.

I'm not sure how many people actually care to enforce that "you only prefix a parameter with an underscore if it's unused", vs just allowing it all the time.

tsutils does have the 1k line usage detector utility (which the original rule uses), but there's probably a decent perf impact do doing that for every arg/var as well.

Something that can easily be added in later.

@bradzacher bradzacher marked this pull request as ready for review Dec 11, 2019
@bradzacher bradzacher force-pushed the naming-conventions branch 2 times, most recently from 278a25a to 8e0f14c Dec 11, 2019
@glen-84

This comment has been minimized.

Copy link

glen-84 commented Dec 11, 2019

Unfortunately not, because JSON Schema doesn't allow for RegExp objects.

Oh, that's unfortunate. Anyway, you did end up closing it. 😄

I think for the first cut, probably not.

Fair enough. Once this is merged, I'll update #816.

The prefix/suffix option only allows exact strings. I was thinking i'd change it to be an object

If you used prefix: [{affix: "I", opt: "forbid"}], then wouldn't it ban anything starting with an I, even when it's not followed by another upper-case letter?

If you're also requiring PascalCase, then I suppose it could "look ahead" and see that after stripping the I from Include, nclude is not PascalCase.

It should allow all of your examples.

My regex actually shouldn't have included the 3rd [A-Z], that was a mistake I think. So the differences would be:

a) The regex limits it to one word (TKey, but not TKeyAndMore).
b) The regex allows any single prefix character (A-Z), and not just those listed as prefixes.

Not really a big deal for me, but as I said, I have no idea what strange naming conventions other developers will have. I guess we'll find out soon enough. 🙂

I'm not sure how many people actually care to enforce that "you only prefix a parameter with an underscore if it's unused", vs just allowing it all the time.

It would bug me if developers used underscore-prefixed parameters, when all/most other identifiers were strictCamelCase. It also wouldn't be obvious which parameters were unused, if you couldn't trust that underscores were only allowed on unused parameters. It's the same reason for having the backingField option.

@bradzacher

This comment has been minimized.

Copy link
Member Author

bradzacher commented Dec 11, 2019

If you used prefix: [{affix: "I", opt: "forbid"}], then wouldn't it ban anything starting with an I, even when it's not followed by another upper-case letter?

Hmmm, good point. I'll have to think on this more, damn.

I have no idea what strange naming conventions other developers will have. I guess we'll find out soon enough. 🙂

That's my plan. I'm hoping that releasing this will bring all of the weird people out of the woodwork to give concrete examples of wanting very specific naming conventions.

It would bug me if developers used underscore-prefixed parameters, when all/most other identifiers were strictCamelCase.

Yeah I get you.
I wish there was an easy way to do this without using that 1k line library. Unfortunately (as I learned from doing no-unused-vars-experimental), typescript doesn't report any diagnostics for underscore-prefixed names, so I can't even rely on reusing that.

I.e. using inbuilt TS logic, I can enforce that unused vars must have an underscore, but I can't enforce that used vars do not have an underscore.

The only option for doing it is the manual analysis 😢

@glen-84

This comment has been minimized.

Copy link

glen-84 commented Dec 11, 2019

typescript doesn't report any diagnostics for underscore-prefixed names

Is it worth submitting an issue to the TS project about this?

bradzacher added 10 commits Dec 9, 2019
@armano2 armano2 dismissed their stale review Jan 2, 2020

:)

bradzacher added 4 commits Jan 3, 2020
bradzacher added 4 commits Jan 9, 2020
@bradzacher

This comment has been minimized.

Copy link
Member Author

bradzacher commented Jan 9, 2020

note: added option custom.
This will let you enforce that an identifier matches(/doesn't match) a regex, which means you can ban prefixes.

This was the last blocker I had for this rule, so if everyone is happy, I'm good to merge this.

bradzacher added 3 commits Jan 13, 2020
@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 13, 2020

Codecov Report

Merging #1318 into master will decrease coverage by 0.35%.
The diff coverage is 87.95%.

@@            Coverage Diff             @@
##           master    #1318      +/-   ##
==========================================
- Coverage   94.45%   94.09%   -0.36%     
==========================================
  Files         142      143       +1     
  Lines        6100     6457     +357     
  Branches     1736     1823      +87     
==========================================
+ Hits         5762     6076     +314     
- Misses        183      204      +21     
- Partials      155      177      +22
Impacted Files Coverage Δ
...s/eslint-plugin/src/rules/interface-name-prefix.ts 83.33% <ø> (ø) ⬆️
packages/eslint-plugin/src/rules/member-naming.ts 100% <ø> (ø) ⬆️
packages/eslint-plugin/src/rules/camelcase.ts 92.85% <ø> (ø) ⬆️
...kages/eslint-plugin/src/rules/class-name-casing.ts 100% <ø> (ø) ⬆️
...ges/eslint-plugin/src/rules/generic-type-naming.ts 100% <ø> (ø) ⬆️
packages/eslint-plugin/src/util/misc.ts 92.59% <100%> (+0.59%) ⬆️
packages/eslint-plugin/src/rules/index.ts 100% <100%> (ø) ⬆️
...kages/eslint-plugin/src/rules/naming-convention.ts 87.85% <87.85%> (ø)
@bradzacher

This comment has been minimized.

Copy link
Member Author

bradzacher commented Jan 13, 2020

I'll merge this in before the release tomorrow morning (in like 13 hours).
So if anyone has any changes, LMK before then.

It's a rather large rule with a lot of deprecations, so I just want to make sure everyone agrees its in the right spot before it goes in.

@bradzacher bradzacher merged commit 9eab26f into master Jan 13, 2020
6 of 7 checks passed
6 of 7 checks passed
codecov/patch 87.95% of diff hit (target 90%)
Details
Semantic Pull Request ready to be squashed
Details
codecov/project 94.09% (-0.36%) compared to 25092fd
Details
typescript-eslint.typescript-eslint Build #20200113.11 succeeded
Details
typescript-eslint.typescript-eslint (Primary code validation and tests) Primary code validation and tests succeeded
Details
typescript-eslint.typescript-eslint (Run unit tests on other Node.js versions node_10_x) Run unit tests on other Node.js versions node_10_x succeeded
Details
typescript-eslint.typescript-eslint (Run unit tests on other Node.js versions node_8_x) Run unit tests on other Node.js versions node_8_x succeeded
Details
@bradzacher bradzacher deleted the naming-conventions branch Jan 13, 2020
@ulrichb

This comment has been minimized.

Copy link
Contributor

ulrichb commented Jan 14, 2020

... after updating to 2.16 I get:

Error: .eslintrc.js#overrides[0] » plugin:@typescript-eslint/all:
	Configuration for rule "@typescript-eslint/naming-convention" is invalid:
	Value [] should NOT have fewer than 1 items.

.. and I also cannot disable it in my project via "@typescript-eslint/naming-convention": "off"

@yenshih

This comment has been minimized.

Copy link

yenshih commented Jan 14, 2020

Got the same error when using plugin:@typescript-eslint/all.

Error: .eslintrc.json » plugin:@typescript-eslint/all:
        Configuration for rule "@typescript-eslint/naming-convention" is invalid:
        Value [] should NOT have fewer than 1 items.

https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/src/configs/all.json#L23

@oleg-koval

This comment has been minimized.

Copy link

oleg-koval commented Jan 14, 2020

Same here:

plugin:@typescript-eslint/all:
        Configuration for rule "@typescript-eslint/naming-convention" is invalid:
        Value [] should NOT have fewer than 1 items.
@viestat

This comment has been minimized.

Copy link

viestat commented Jan 14, 2020

Also here:

plugin:@typescript-eslint/all:
        Configuration for rule "@typescript-eslint/naming-convention" is invalid:
        Value [] should NOT have fewer than 1 items.
@bradzacher

This comment has been minimized.

Copy link
Member Author

bradzacher commented Jan 14, 2020

Hey guys, thanks for the reports.
I'll submit a fix for this.

Just a heads up for how we like to do things around here:

  • Please don't comment on closed PRs. Closed PRs don't show up in the default issue search, and are hard to track.
    • Please open a new issue. An issue has a trackable lifecycle, and can be pinned to increase visibility, etc.
  • Also, please avoid commenting the exact same comment. If you have the same issues, reactions will do perfectly. The github inbox only shows 1 line per PR/issue, so more of the same comment doesn't increase visibility for maintainers.
@bradzacher

This comment has been minimized.

Copy link
Member Author

bradzacher commented Jan 14, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.