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] fix precedence of method and property meta selectors #2877

Conversation

susisu
Copy link
Contributor

@susisu susisu commented Dec 15, 2020

Fixes #2860

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @susisu!

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.

@susisu susisu force-pushed the fix-naming-conventionn-selector-precedence branch from 7d80a2c to 5fd2fcb Compare December 15, 2020 09:43
@codecov
Copy link

codecov bot commented Dec 15, 2020

Codecov Report

Merging #2877 (77c36ce) into master (d35a539) will decrease coverage by 0.01%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #2877      +/-   ##
==========================================
- Coverage   92.77%   92.76%   -0.02%     
==========================================
  Files         310      310              
  Lines       10367    10393      +26     
  Branches     2934     2943       +9     
==========================================
+ Hits         9618     9641      +23     
- Misses        346      347       +1     
- Partials      403      405       +2     
Flag Coverage Δ
unittest 92.76% <83.33%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
...gin/src/rules/naming-convention-utils/validator.ts 94.40% <71.42%> (-1.08%) ⬇️
...plugin/src/rules/naming-convention-utils/shared.ts 100.00% <100.00%> (ø)
.../eslint-plugin/src/rules/promise-function-async.ts 97.36% <0.00%> (-2.64%) ⬇️
...gin/src/rules/non-nullable-type-assertion-style.ts 100.00% <0.00%> (ø)

@susisu
Copy link
Contributor Author

susisu commented Dec 15, 2020

@bradzacher Should I also update the docs for this?

@bradzacher bradzacher added the bug Something isn't working label Dec 16, 2020
@bradzacher
Copy link
Member

Should I also update the docs for this

No need - you're fixing a regression, so the docs should already match this behaviour.

@susisu
Copy link
Contributor Author

susisu commented Dec 17, 2020

No need - you're fixing a regression, so the docs should already match this behaviour.

OK, thanks.

Comment on lines 56 to 63
// for backward compatibility, method and property have higher precedence than other meta selectors
if (isMethodOrPropertySelector(a.selector)) {
return -1;
}
if (isMethodOrPropertySelector(b.selector)) {
return 1;
}

Copy link
Contributor Author

@susisu susisu Dec 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ordering can be modified by adding a flag bit to method and property selectors, as suggested in #2860 (comment).
But I think it's bit tricky, and I implemented a simpler one.
This should work well as long as method and property are the only exceptional cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to special case this here. It's the easiest way to handle it. We can always expand on it later if we need to!


This is almost correct!
This logic will create an unstable ordering when both selectors are method/property - but both aren't the same selector.

If both are method/property (and not the same selector), then the order will be the order as written.

This is technically "consistent" for the most part as config rarely changes, but if someone changes their config and reorders the config then all of a sudden the rule could produce a different result - which will be super confusing and hard to figure out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I missed that case... Thanks!
Fixed in 77c36ce

@susisu susisu marked this pull request as ready for review December 17, 2020 10:26
Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far LGTM - almost there!
Just one comment.
Thanks for this!

Comment on lines 56 to 63
// for backward compatibility, method and property have higher precedence than other meta selectors
if (isMethodOrPropertySelector(a.selector)) {
return -1;
}
if (isMethodOrPropertySelector(b.selector)) {
return 1;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to special case this here. It's the easiest way to handle it. We can always expand on it later if we need to!


This is almost correct!
This logic will create an unstable ordering when both selectors are method/property - but both aren't the same selector.

If both are method/property (and not the same selector), then the order will be the order as written.

This is technically "consistent" for the most part as config rarely changes, but if someone changes their config and reorders the config then all of a sudden the rule could produce a different result - which will be super confusing and hard to figure out.

@bradzacher bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Dec 21, 2020
@susisu susisu force-pushed the fix-naming-conventionn-selector-precedence branch from 047afb8 to 77c36ce Compare December 21, 2020 11:00
@bradzacher bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Dec 21, 2020
Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@bradzacher bradzacher merged commit 2f10e1a into typescript-eslint:master Dec 27, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[naming-convention] Precedence of property selector has been changed
2 participants