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): addallowedPrefixes prop tointerface-name-prefix#1433

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

Closed
G-Rath wants to merge1 commit intotypescript-eslint:masterfromG-Rath:add-allowedNames-property-to-interface-name-prefix
Closed

feat(eslint-plugin): addallowedPrefixes prop tointerface-name-prefix#1433

G-Rath wants to merge1 commit intotypescript-eslint:masterfromG-Rath:add-allowedNames-property-to-interface-name-prefix

Conversation

G-Rath
Copy link
Contributor

I hit this when working on my Terraform generator/parser/importer, where I have interfaces calledIAM.

I've addedIAM as the default value, but can remove that - it's the only real exception I could think of that made sense, but there could be more.

Also happy to not make it an option and just hardcode it instead 🤷‍♂

@typescript-eslint
Copy link
Contributor

Thanks for the PR,@G-Rath!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!


🙏Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently onhttps://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitorsper day.

@codecov
Copy link

codecovbot commentedJan 12, 2020
edited
Loading

Codecov Report

Merging#1433 intomaster willincrease coverage by0.06%.
The diff coverage is100%.

@@            Coverage Diff             @@##           master    #1433      +/-   ##==========================================+ Coverage   94.48%   94.55%   +0.06%==========================================  Files         142      142                Lines        6077     6076       -1       Branches     1726     1726              ==========================================+ Hits         5742     5745       +3+ Misses        182      180       -2+ Partials      153      151       -2
Impacted FilesCoverage Δ
...s/eslint-plugin/src/rules/interface-name-prefix.ts100% <100%> (+16.66%)⬆️

@G-RathG-Rath changed the titlefeat(eslint-plugin): addallowedNames prop tointerface-name-prefixfeat(eslint-plugin): addallowedPrefixes prop tointerface-name-prefixJan 12, 2020
@G-RathG-Rath requested a review fromarmano2January 12, 2020 02:20
@armano2
Copy link
Collaborator

armano2 commentedJan 12, 2020
edited
Loading

@G-Rath you don't have force push commits as we always squash changes before merging (all other merge methods are disabled in this repo)

(this helps with tracking what review comments was addressed in commit)

@G-Rath
Copy link
ContributorAuthor

@armano2 cheers for the reminder - I've been bouncing between a bunch of OSS repos, so my brains been autopiloting a bit 😂

Thanks btw for the very good example of PM vs AM - I completely didn't think of it.

armano2 reacted with thumbs up emoji

@armano2armano2 added enhancement: plugin rule optionNew rule option for an existing eslint-plugin rule package: eslint-pluginIssues related to @typescript-eslint/eslint-plugin labelsJan 12, 2020
@bradzacher
Copy link
Member

Thanks for submitting this, but this rule is about to be deprecated.
See#1318, the new rule will handle cases like this in a much nicer fashion.

@armano2
Copy link
Collaborator

@bradzacher we do not know when#1318 will be ready, and this one can be good enough as temp fix while ppl are waiting.

@bradzacher
Copy link
Member

#1318 is ready, I just need to fix up the recommended config tests/generator so that it doesn't remove deprecated rules.
I was planning on having it in with the next release.

armano2 reacted with thumbs up emoji

@bradzacher
Copy link
Member

naming-convention is released, so I'm going to close this.
thanks for the contribution!

@G-Rath
Copy link
ContributorAuthor

G-Rath commentedJan 19, 2020
edited
Loading

@bradzacher hate to bother you with this, but would you be able to help me form a regex usingnaming-convention that allowsIAM but notI[A-Z]?

The example you provided in the original PR works perfectly as a replacement forinterface-name-prefix:

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

But I can't come up with a regex that doesn't match IAM but still matches otherI prefixes.

(I'm having trouble in general figuring out how to make exclusions work, such as to allowchild_process, but I'll probably make a proper issue to track that).

@bradzacher
Copy link
Member

For reference; theinterface-name-prefix rule didn't work in this case either:

image

You could go with a more complex regex to support multiple cases. This will work if your node version supports negative look-behinds.

/^([A-Z](?<!I)\w+|I[A-Z]+)$/   [A-Z](?<!I)\w+   ^^^^^^^^^^^^^^ use a negative look-behind to ban I prefix                  I[A-Z]+                  ^^^^^^^ match I followed by all caps (IAM)

Make surematch: true if you use that regex.

https://regexr.com/4sjvl

@G-Rath
Copy link
ContributorAuthor

For reference; the interface-name-prefix rule didn't work in this case either:

Which is why I made this PR :)

/^(A-Z\w+|I[A-Z]+)$/

Amazing thanks!

I had a feeling negative look-behinds would be the case - playing around with that regexp, to me it seems like that might actually be the The Right One for that rule? (not that it matters since it's deprecated) - I can't think of a combo that generates a false positive.

Might be worth sticking somewhere - maybe a block under the deprecation notice forinterface-name-prefix saying something like "you can achieve the desired behavior using this "?


Regardless, thanks again 😄

@G-RathG-Rath deleted the add-allowedNames-property-to-interface-name-prefix branchJanuary 21, 2020 02:22
@github-actionsgithub-actionsbot locked asresolvedand limited conversation to collaboratorsApr 20, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@armano2armano2Awaiting requested review from armano2

Assignees
No one assigned
Labels
enhancement: plugin rule optionNew rule option for an existing eslint-plugin rulepackage: eslint-pluginIssues related to @typescript-eslint/eslint-plugin
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@G-Rath@armano2@bradzacher

[8]ページ先頭

©2009-2025 Movatter.jp