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

tools: add sort-keys rule in eslint #41768

Closed
wants to merge 1 commit into from

Conversation

MrJithil
Copy link
Member

@MrJithil MrJithil commented Jan 30, 2022

Add sort-keys rule in EsLint.
Related issue: #41767

CC: @nodejs/linting

Add sort-keys rule in EsLint.
Related issue: nodejs#41767

CC:@nodejs/linting
@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Jan 30, 2022
@MrJithil
Copy link
Member Author

CC: @nodejs/linting

@aduh95
Copy link
Contributor

aduh95 commented Jan 30, 2022

I agree with the sentiment, the problem being the diff would be way too large for it to be worth it. Ideally we would have another set of "aspirational" lint rules that would apply only on code being added, but I'm not sure it exists a tool that would allow us to do that.

@bricss
Copy link

bricss commented Jan 30, 2022

It might be better to set it as warn-ing ⚠️

@aduh95
Copy link
Contributor

aduh95 commented Jan 30, 2022

Warnings wouldn't help much in our case, it would make ESLint output very noisy, and the auto-fixes would probably still produce that large diff I'd like to avoid.

@MrJithil
Copy link
Member Author

Instead, can we go for error and run --fix for them?

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Thanks for this (and all your recent work it's appreciated!)

Blocking so this doesn't land without consensus which we didn't have last time.

@MrJithil
Copy link
Member Author

MrJithil commented Jan 30, 2022

Thanks for this (and all your recent work it's appreciated!)

Blocking so this doesn't land without consensus which we didn't have last time.

Sure. Let's invite opinions. Thanks @benjamingr

@aduh95
Copy link
Contributor

aduh95 commented Jan 30, 2022

Instead, can we go for error and run --fix for them?

I think it's the same consensus as regarding trailing commas: we'd like to have them used everywhere, but it's not worth accepting PRs focusing on adding them. While improving the consistency of the code base is great, landing commits that do stylistic changes pollute the git blame output, without improving the quality of the codebase otherwise.

If you are able to limit the scope to a smaller chunk of the code base that already complies with this rule, sure that would certainly be accepted, but you have to keep in mind that humans review the PRs, so a PR that introduces thousands of changes across the whole codebase is probably too much to ask.

@targos
Copy link
Member

targos commented Jan 30, 2022

Ideally we would have another set of "aspirational" lint rules that would apply only on code being added

Even if we had that, I would be -1 on enabling this rule with it.
In most cases I don't see any advantage of sorting keys in objects, while using a different order often makes sense (to group related things, to order by importance, to use the same order as other things, etc.).

@MrJithil
Copy link
Member Author

Closing the MR for the time being as discussed the unintended issues behind this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants