fix(eslint-plugin): [naming-convention] fix precedence of method and property meta selectors #2877
Conversation
|
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. |
… precedence than other meta selctors
7d80a2c to
5fd2fcb
Compare
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
@bradzacher Should I also update the docs for this? |
No need - you're fixing a regression, so the docs should already match this behaviour. |
OK, thanks. |
| // 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; | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh, I missed that case... Thanks!
Fixed in 77c36ce
| // 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; | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
047afb8 to
77c36ce
Compare
Fixes #2860