Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
bradzacher merged 36 commits intomasterfromnaming-conventions
Jan 13, 2020

Conversation

bradzacher
Copy link
Member

@bradzacherbradzacher commentedDec 9, 2019
edited
Loading

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

This is a simplified implementation of thenaming-conventions rule fromtslint-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}},],}

chapterjason, ardevelop, armano2, noku, usernamehw, nmalancea, ken0x0a, natterstefan, and ifiokjr reacted with thumbs up emojiglen-84, phaux, and ken0x0a reacted with hooray emojiken0x0a reacted with heart emoji
@bradzacherbradzacher added the enhancement: new plugin ruleNew rule request for eslint-plugin labelDec 9, 2019
@glen-84
Copy link
Contributor

Thanks for working on this! 🙂

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

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

    • strictCamelCase instead ofcamelCase.
    • No leading underscores on private/protected properties.
    • Not sure about thetoJSON exception ... see question below about external code.
    • NoI 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 haveimport {GraphQLResolveInfo} from "graphql";, can I ignore the non-strict naming ofGraphQLResolveInfo because it's not my code?
  3. Is your example outdated? Not sure what thevariableLike/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
Copy link
MemberAuthor

bradzacher commentedDec 10, 2019
edited
Loading

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 alot of code to track usages, so I don't think it's worth adding. (esp when so many people use theno-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 forconst x = ... vsconst { 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 useprefer-const, which has a fixer, and would cause no end of naming annoyance when the auto fixer changeslet toconst.

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 hadparameterProperty which extendedproperty which extendedmember which extendeddefault).
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 afinal 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, andtypeLike), but they are just a simple shortcut (variableLike is the same as writing the selector three times, once forvariable,function andparameter).

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 toban prefixes (right now it only lets you assert there is one of a set). Thinking about adding this in (so I can completely deprecateinterface-name-prefix).

Not sure about the toJSON exception

ThetoJSON exception is becausetoJSON is a special method which is called on objects when traversed byJSON.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
Copy link
Contributor

glen-84 commentedDec 10, 2019
edited
Loading

  • unused – I usenoUnusedParameters, 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:naming-convention: allow detecting unused variables / parameters 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 fromproperty, which inherits its configuration frommember, which inherits its configuration fromdefault.

i.e. If you don't override settings forparameterProperty, it gets the same settings asproperty.

Andfinal meant that a configuration should not affect "subtypes", for example if you define a rule forproperty, making itfinal 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+)?$ forgeneric-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
Copy link
Contributor

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

IfleadingUnderscore 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.

@bradzacher
Copy link
MemberAuthor

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/setterand 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+)?$ forgeneric-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.

@bradzacherbradzacher marked this pull request as ready for reviewDecember 11, 2019 04:09
@bradzacherbradzacherforce-pushed thenaming-conventions branch 2 times, most recently from278a25a to8e0f14cCompareDecember 11, 2019 06:41
@glen-84
Copy link
Contributor

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 usedprefix: [{affix: "I", opt: "forbid"}], then wouldn't it ban anything starting with anI, 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 theI fromInclude,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 notTKeyAndMore).
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 thebackingField option.

@bradzacher
Copy link
MemberAuthor

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 doingno-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
Copy link
Contributor

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

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

@bradzacherbradzacher added the DO NOT MERGEPRs which should not be merged yet labelJan 7, 2020
@bradzacher
Copy link
MemberAuthor

note: added optioncustom.
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.

glen-84 and ardevelop reacted with thumbs up emojichapterjason and ardevelop reacted with rocket emoji

@codecov
Copy link

codecovbot commentedJan 13, 2020

Codecov Report

Merging#1318 intomaster willdecrease coverage by0.35%.
The diff coverage is87.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 FilesCoverage Δ
...s/eslint-plugin/src/rules/interface-name-prefix.ts83.33% <ø> (ø)⬆️
packages/eslint-plugin/src/rules/member-naming.ts100% <ø> (ø)⬆️
packages/eslint-plugin/src/rules/camelcase.ts92.85% <ø> (ø)⬆️
...kages/eslint-plugin/src/rules/class-name-casing.ts100% <ø> (ø)⬆️
...ges/eslint-plugin/src/rules/generic-type-naming.ts100% <ø> (ø)⬆️
packages/eslint-plugin/src/util/misc.ts92.59% <100%> (+0.59%)⬆️
packages/eslint-plugin/src/rules/index.ts100% <100%> (ø)⬆️
...kages/eslint-plugin/src/rules/naming-convention.ts87.85% <87.85%> (ø)

@bradzacher
Copy link
MemberAuthor

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.

ardevelop, glen-84, and chapterjason reacted with thumbs up emoji

@bradzacherbradzacher merged commit9eab26f intomasterJan 13, 2020
@bradzacherbradzacher deleted the naming-conventions branchJanuary 13, 2020 17:23
@ulrichb
Copy link
Contributor

ulrichb commentedJan 14, 2020
edited
Loading

... 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"

oleg-koval and yenshih reacted with thumbs up emojioleg-koval reacted with eyes emoji

@yenshih
Copy link

Got the same error when usingplugin:@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 reacted with eyes emoji

@oleg-koval
Copy link

Same here:

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

@viestat
Copy link

Also here:

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

@bradzacher
Copy link
MemberAuthor

bradzacher commentedJan 14, 2020
edited
Loading

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.
viestat, lsnch, and haltcase reacted with thumbs up emoji

@bradzacher
Copy link
MemberAuthor

#1455

Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@glen-84glen-84glen-84 requested changes

@armano2armano2armano2 left review comments

Assignees
No one assigned
Labels
enhancement: new plugin ruleNew rule request for eslint-plugin
Projects
None yet
Milestone
No milestone
7 participants
@bradzacher@glen-84@ulrichb@yenshih@oleg-koval@viestat@armano2

[8]ページ先頭

©2009-2025 Movatter.jp