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

fix(eslint-plugin): [naming-convention] allow an array of selectors with types and modifiers #2415

Merged
merged 5 commits into from Sep 14, 2020

Conversation

@ginger-kang
Copy link
Contributor

@ginger-kang ginger-kang commented Aug 22, 2020

Fixes #2376

Please let me know if there are any required changes!

@typescript-eslint
Copy link

@typescript-eslint typescript-eslint bot commented Aug 22, 2020

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.

@bradzacher bradzacher added the bug label Aug 23, 2020
Copy link
Member

@bradzacher bradzacher left a comment

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:

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:

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

@codecov codecov bot commented Aug 25, 2020

Codecov Report

Merging #2415 into master will decrease coverage by 0.31%.
The diff coverage is 100.00%.

@@            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     
Flag Coverage Δ
#unittest 92.82% <100.00%> (-0.32%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...kages/eslint-plugin/src/rules/naming-convention.ts 89.30% <100.00%> (+0.14%) ⬆️
...n/src/rules/no-non-null-asserted-optional-chain.ts 41.93% <0.00%> (-33.07%) ⬇️
...ackages/eslint-plugin/src/rules/no-var-requires.ts 88.88% <0.00%> (-11.12%) ⬇️
packages/eslint-plugin/src/rules/semi.ts 92.30% <0.00%> (-7.70%) ⬇️
...ackages/eslint-plugin/src/rules/keyword-spacing.ts 92.85% <0.00%> (-7.15%) ⬇️
...kages/eslint-plugin/src/rules/no-empty-function.ts 77.14% <0.00%> (-6.65%) ⬇️
...nt-plugin/src/rules/lines-between-class-members.ts 85.71% <0.00%> (-6.60%) ⬇️
packages/eslint-plugin/src/rules/quotes.ts 88.88% <0.00%> (-5.23%) ⬇️
...s/eslint-plugin/src/rules/no-unused-expressions.ts 88.88% <0.00%> (-5.23%) ⬇️
...kages/eslint-plugin/src/rules/init-declarations.ts 76.47% <0.00%> (-4.78%) ⬇️
... and 49 more
Copy link
Member

@bradzacher bradzacher left a comment

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)
Comment on lines 1317 to 1324

This comment has been minimized.

@bradzacher

bradzacher Sep 6, 2020
Member

nit - use the selector enum instead of redefining the numbers.

Suggested change
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)
@ginger-kang
Copy link
Contributor Author

@ginger-kang ginger-kang commented Sep 7, 2020

@bradzacher Thanks for review :)
Actually, a type error occurs because the selector in forEach is of Selectors | MetaSelectors type. So, I modified selectorsAllowedToHaveTypes to an array of Selectors type.

Copy link
Member

@bradzacher bradzacher left a comment

LGTM - thanks for your contribution!

@bradzacher bradzacher changed the title fix(eslint-plugin): [naming-convention] Can use array of selectors with types and modifiers fix(eslint-plugin): [naming-convention] allow an array of selectors with types and modifiers Sep 14, 2020
@bradzacher bradzacher merged commit 7ca54c3 into typescript-eslint:master Sep 14, 2020
10 checks passed
10 checks passed
Typecheck
Details
Unit tests Unit tests
Details
Code style and lint
Details
Run integration tests on primary Node.js version
Details
Run unit tests on other Node.js versions (10.x)
Details
Run unit tests on other Node.js versions (14.x)
Details
Publish the latest code as a canary version
Details
Semantic Pull Request ready to be squashed
Details
codecov/patch Codecov Report
Details
codecov/project Codecov Report
Details
phaux added a commit to phaux/typescript-eslint that referenced this pull request Sep 28, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants