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
fix(eslint-plugin): [naming-convention] allow an array of selectors with types and modifiers #2415
fix(eslint-plugin): [naming-convention] allow an array of selectors with types and modifiers #2415
Conversation
Thanks for the PR, @ginger-kang! 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a great start! This fixes the JSON schema to allow the config.
However this still has one slight problem.
If you provide a config like
{
selector: ['variable', 'function'],
types: ['string'],
format: ['PascalCase'],
prefix: ['my', 'My'],
}
Then you will likely get "undefined" behaviour, or rather the rule will probably incorrectly not fail for this case:
function my_foo_bar() {}
This is because it'll check to see if the identifier my_foo_bar
has type string
before it decides if it should validate the name. Obviously the function can never have type string
, so this will never work.
To fix this, we have to change this function here:
typescript-eslint/packages/eslint-plugin/src/rules/naming-convention.ts
Lines 1260 to 1322 in 32fe2bb
function normalizeOption(option: Selector): NormalizedSelector { | |
let weight = 0; | |
option.modifiers?.forEach(mod => { | |
weight |= Modifiers[mod]; | |
}); | |
option.types?.forEach(mod => { | |
weight |= TypeModifiers[mod]; | |
}); | |
// give selectors with a filter the _highest_ priority | |
if (option.filter) { | |
weight |= 1 << 30; | |
} | |
const normalizedOption = { | |
// format options | |
format: option.format ? option.format.map(f => PredefinedFormats[f]) : null, | |
custom: option.custom | |
? { | |
regex: new RegExp(option.custom.regex, 'u'), | |
match: option.custom.match, | |
} | |
: null, | |
leadingUnderscore: | |
option.leadingUnderscore !== undefined | |
? UnderscoreOptions[option.leadingUnderscore] | |
: null, | |
trailingUnderscore: | |
option.trailingUnderscore !== undefined | |
? UnderscoreOptions[option.trailingUnderscore] | |
: null, | |
prefix: option.prefix && option.prefix.length > 0 ? option.prefix : null, | |
suffix: option.suffix && option.suffix.length > 0 ? option.suffix : null, | |
modifiers: option.modifiers?.map(m => Modifiers[m]) ?? null, | |
types: option.types?.map(m => TypeModifiers[m]) ?? null, | |
filter: | |
option.filter !== undefined | |
? typeof option.filter === 'string' | |
? { regex: new RegExp(option.filter, 'u'), match: true } | |
: { | |
regex: new RegExp(option.filter.regex, 'u'), | |
match: option.filter.match, | |
} | |
: null, | |
// calculated ordering weight based on modifiers | |
modifierWeight: weight, | |
}; | |
const selectors = Array.isArray(option.selector) | |
? option.selector | |
: [option.selector]; | |
return { | |
selector: selectors | |
.map(selector => | |
isMetaSelector(selector) | |
? MetaSelectors[selector] | |
: Selectors[selector], | |
) | |
.reduce((accumulator, selector) => accumulator | selector), | |
...normalizedOption, | |
}; | |
} |
As mentioned in #2376 (comment), the PR that added array of selectors went the lazy route, and used a binary OR to join the selectors:
typescript-eslint/packages/eslint-plugin/src/rules/naming-convention.ts
Lines 1313 to 1319 in 32fe2bb
selector: selectors | |
.map(selector => | |
isMetaSelector(selector) | |
? MetaSelectors[selector] | |
: Selectors[selector], | |
) | |
.reduce((accumulator, selector) => accumulator | selector), |
This is okay, but it means we can't validate that the types
matches what we expect - meaning later in the rule we can't rely on the assumption that the config is correct.
Instead of doing a binary OR, if this function returned one config per selector
in the selector
array, then we would have a 1:1 mapping between selector
and types
, allowing us to ignore types
altogether for certain things (like functions).
Codecov Report
@@ Coverage Diff @@
## master #2415 +/- ##
==========================================
- Coverage 93.13% 92.82% -0.32%
==========================================
Files 287 290 +3
Lines 9176 9458 +282
Branches 2523 2648 +125
==========================================
+ Hits 8546 8779 +233
- Misses 302 322 +20
- Partials 328 357 +29
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
almost there - this is looking great!
One last change and this is good to go.
Thanks for your work!
const allowedType = [1 << 0, 1 << 2, 1 << 3, 1 << 4, 1 << 6]; | ||
const config: NormalizedSelector[] = []; | ||
selectors | ||
.map(selector => | ||
isMetaSelector(selector) ? MetaSelectors[selector] : Selectors[selector], | ||
) | ||
.forEach(selector => | ||
allowedType.includes(selector) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - use the selector enum instead of redefining the numbers.
const allowedType = [1 << 0, 1 << 2, 1 << 3, 1 << 4, 1 << 6]; | |
const config: NormalizedSelector[] = []; | |
selectors | |
.map(selector => | |
isMetaSelector(selector) ? MetaSelectors[selector] : Selectors[selector], | |
) | |
.forEach(selector => | |
allowedType.includes(selector) | |
const selectorsAllowedToHaveTypes = new Set<Selector>([ | |
Selectors.variable, | |
Selectors.parameter, | |
Selectors.property, | |
Selectors.parameterProperty, | |
Selectors.accessor, | |
]); | |
const config: NormalizedSelector[] = []; | |
selectors | |
.map(selector => | |
isMetaSelector(selector) ? MetaSelectors[selector] : Selectors[selector], | |
) | |
.forEach(selector => | |
selectorsAllowedToHaveTypes.has(selector) |
@bradzacher Thanks for review :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - thanks for your contribution!
Fixes #2376
Please let me know if there are any required changes!