-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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] order
: codify invariants from docs into config schema
#3152
base: main
Are you sure you want to change the base?
Conversation
This PR seems to have too much stuff in it - but either way, if a rule or option is already released, its schema can’t be tightened further. |
@ljharb It seems like it has a lot because it's on top of the EDIT: I can also rebase onto main and decouple from the other PRs, but not hitting all the new tests plus no direct tests of the schema makes me a bit nervous. As for the tightening of existing options, I assume you're talking about lines 865 and 866 for |
In that case, I'll wait to review this since it needs to wait on #3129 |
42252c4
to
a8ab646
Compare
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.
these changes are non-breaking iff they exclude things that the rule already checks.
… which would also imply there's some code that can be removed, since the checks are not guaranteed to be done by eslint?
src/rules/order.js
Outdated
@@ -858,9 +858,12 @@ module.exports = { | |||
properties: { | |||
groups: { | |||
type: 'array', | |||
// Verified manually in the convertGroupsToRanks function |
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.
no need for the comment. but this can be verified to have numbers in it, and no duplicates, yes?
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.
The secondary validation has its own error messages. I believe you're saying to move the duplicates/existence validation logic up to the schema? That would change the error messages and perhaps other behavior. For instance, there are currently unit tests that check for the current error messages.
Would that count as tightening the schema of a released option?
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.
Changed error messages are fine; test cases that always error, changing to "removed test cases because it's now unreachable" is also fine, I think.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3152 +/- ##
==========================================
- Coverage 95.17% 92.19% -2.98%
==========================================
Files 83 83
Lines 3688 3690 +2
Branches 1331 1331
==========================================
- Hits 3510 3402 -108
- Misses 178 288 +110 ☔ View full report in Codecov by Sentry. |
…; undo cosmetic changes
Updating schema wrt #3127 (comment)
EDIT: depends on #3129 for its added tests