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

[ginger-kang]

Fixes #2376

Please let me know if there are any required changes!

[typescript-eslint]

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]

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]

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

[bradzacher]

almost there - this is looking great!
One last change and this is good to go.
Thanks for your work!

[bradzacher]

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]

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

[bradzacher]

LGTM - thanks for your contribution!