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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(eslint-plugin): add allowedPrefixes prop to interface-name-prefix #1433

Closed
wants to merge 1 commit into from
Closed

feat(eslint-plugin): add allowedPrefixes prop to interface-name-prefix #1433

wants to merge 1 commit into from

Conversation

G-Rath
Copy link
Contributor

@G-Rath G-Rath commented Jan 12, 2020

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

I've added IAM 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 on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

@codecov
Copy link

codecov bot commented Jan 12, 2020

Codecov Report

Merging #1433 into master will increase coverage by 0.06%.
The diff coverage is 100%.

@@            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 Files Coverage 螖
...s/eslint-plugin/src/rules/interface-name-prefix.ts 100% <100%> (+16.66%) 猬嗭笍

@G-Rath G-Rath changed the title feat(eslint-plugin): add allowedNames prop to interface-name-prefix feat(eslint-plugin): add allowedPrefixes prop to interface-name-prefix Jan 12, 2020
@G-Rath G-Rath requested a review from armano2 January 12, 2020 02:20
@armano2
Copy link
Member

armano2 commented Jan 12, 2020

@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
Contributor Author

G-Rath commented Jan 12, 2020

@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 armano2 added enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin labels Jan 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
Member

armano2 commented Jan 12, 2020

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

@bradzacher
Copy link
Member

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

@bradzacher bradzacher closed this Jan 16, 2020
@G-Rath
Copy link
Contributor Author

G-Rath commented Jan 19, 2020

@bradzacher hate to bother you with this, but would you be able to help me form a regex using naming-convention that allows IAM but not I[A-Z]?

The example you provided in the original PR works perfectly as a replacement for interface-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 other I prefixes.

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

@bradzacher
Copy link
Member

For reference; the interface-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 sure match: true if you use that regex.

https://regexr.com/4sjvl

@G-Rath
Copy link
Contributor Author

G-Rath commented Jan 21, 2020

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 for interface-name-prefix saying something like "you can achieve the desired behavior using this "?


Regardless, thanks again 馃槃

@G-Rath G-Rath deleted the add-allowedNames-property-to-interface-name-prefix branch January 21, 2020 02:22
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants