Fix: sort-keys in an object that contains spread (fixes #10261) #10495
Conversation
|
|
eslint-deprecated
bot
added
the
triage
katerberg
force-pushed
the
master
branch
3 times, most recently
from
f354459
to
90c92b4
Compare
There was a problem hiding this comment.
Code changes LGTM, thanks!
Just two suggestions:
- Could the commit message mention the sort-keys rule by name? This will allow it to automatically link to the rule docs in our changelog and release notes.
- I think it would be good to add an extra example to the sort-keys rule docs about how object spread means no errors will be reported. Would you mind looking into that? Let us know if you need guidance.
Thanks for contributing!
|
@katerberg Friendly ping... Did you have any questions about my review, and/or is there anything else I can do to help? Thanks! |
platinumazure
added
bug
|
Hmm, why would we ignore all sorting? I'd think that the keys would be split up into groups - with a spread as the boundary - and each group would be individually sorted. |
|
@platinumazure I'm still going to do this, but have had a busy weekend. Hoping to get to it next weekend. @ljharb That would be reasonable as well. Do you and @platinumazure agree that it should be the behavior? If so, I can change the PR to match those goals. |
|
@katerberg I'm okay with trying to report on sorting keys in the "groups" before and after the object rest property node, if that seems feasible to you. I can't remember if the rule currently does autofix or not, but if it does, I'll note that rules do not always have to autofix if they autofix in other cases: Users know that autofix is not guaranteed. So basically, I'm suggesting not to bite off too much with this bugfix. Incremental improvements are totally okay here. Thanks again for contributing! |
|
Sounds good. I'll likely keep it as it is for now and make an issue to add the grouped alphabetizing as a separate PR. It looks like autofix is off for this rule on purpose: #10324 |
katerberg
force-pushed
the
master
branch
2 times, most recently
from
dac2702
to
01ca9fa
Compare
|
I've now added the documentation to make the fact that it ignores objects with spread operators in them and updated the commit message. Let me know if you'd like anything else! |
kaicataldo
changed the title
not-an-aardvark
dismissed
platinumazure’s stale review
Comments have been addressed
eslint-deprecated
bot
added
the
archived due to age
What is the purpose of this pull request? (put an "X" next to item)
[ ] Documentation update
[X] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
I caused the linter to ignore any key-ordering in an object that contains a spread operator. Previously, it would verify that any keys that were not themselves spread operators were in the correct order, but this caused issues when the spread operator was between other keys as illustrated in https://eslint.org/docs/developer-guide/contributing/pull-requests
Is there anything you'd like reviewers to focus on?
This is my first pull request to this project, so any feedback would be welcomed. Since this does remove a test, I would also like to have a second set of eyes verify that the original test was indeed accidental and should have been reversed.