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: use stylistic ESLint plugin for formatting #50714

Merged
merged 1 commit into from
Apr 15, 2024

Conversation

targos
Copy link
Member

@targos targos commented Nov 13, 2023

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/http2
  • @nodejs/net
  • @nodejs/security-wg
  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Nov 13, 2023
@targos targos mentioned this pull request Nov 13, 2023
3 tasks
Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

Even though, the proposed change solves the current deprecated rules issue, I strongly believe that it should be interpreted as a "wake up call" to resolve the formatting issues in the repository.

None of these rules fixes the formatting issues related to the examples I've mentioned in the original pull request.

@targos
Copy link
Member Author

targos commented Nov 13, 2023

I strongly believe that it should be interpreted as a "wake up call" to resolve the formatting issues in the repository.

This is orthogonal. We can fix the immediate deprecation AND work on a replacement.

@anonrig
Copy link
Member

anonrig commented Nov 13, 2023

This is orthogonal. We can fix the immediate deprecation AND work on a replacement.

The deprecated rules will not be removed until ESLint 10 (according to the blog post announcing the deprecation). So there is no immediate action needs to be taken as of today. If we might/will/can replace the Stylistic package with something else, I think we should keep the git history clean and make the decision right now.

@bricss
Copy link

bricss commented Nov 13, 2023

@anonrig Why there is a need to replace Stylistic package with something else? What's the urgency?
Is it radioactive ☢️, toxic ☣️ or something? 🤔

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

Can we do something similar to facebook/react#4034 and create aliases for the style rules without the prefix? Then we don't even need to update the eslint-disable comments.

@joyeecheung
Copy link
Member

joyeecheung commented Nov 14, 2023

None of these rules fixes the formatting issues related to the examples I've mentioned in the original pull request.

FWIW I don't see those as issues. Whitespace styles can be enforced on a best-effort basis. Not indenting some function call the way some people think it should is not going to do much damage. If we can use a tool that places whitespaces in a less controversial way so that we can stop making it even a topic in code reviews, that's great. To me it doesn't really matter how complete the tool is at finding and enforcing whitespace styles, what's important is that we can just say "the tool doesn't complain" and move on and not spend time bikeshedding about whitespace styles. If the introduction of this tool creates a huge amount of diffs in the existing code base and make whitespaces any more attention-worthy than they already are in diffs, or introduce any bikeshedding about whitespace styles, then I think that's not really worth it.

@targos
Copy link
Member Author

targos commented Nov 14, 2023

@joyeecheung I've done the work already.

Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

LGTM.

@aduh95
Copy link
Contributor

aduh95 commented Nov 15, 2023

@joyeecheung I've done the work already.

I might still be worth to undo that work to minimize the git diff / not pollute the git blame, and create aliases as Joyee suggested. Let us know if you'd prefer someone else do it.

@targos
Copy link
Member Author

targos commented Nov 15, 2023

I certainly don't want to do it while the PR is blocked, and I would prefer if someone else does it.

@bricss
Copy link

bricss commented Nov 15, 2023

IMHO, it's better to keep those rules as is right now, so that people won't get confused from where those rules are came from 🙄

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM

@targos targos added the blocked PRs that are blocked by other issues or PRs. label Dec 4, 2023
@tniessen tniessen removed the blocked PRs that are blocked by other issues or PRs. label Feb 28, 2024
@tniessen
Copy link
Member

I don't think the blocked label applies here. Joyee has made a great suggestion to minimize the size of the diff, and the only thing standing in the way of applying that suggestion and merging this PR in order to fix a specific issue in a minimal manner is one collaborator's objection.

@targos
Copy link
Member Author

targos commented Feb 28, 2024

I considered it blocked on the decision wrt. Biome, not on Joyee's suggestion.

@targos
Copy link
Member Author

targos commented Apr 11, 2024

ESLint v9 was released. What are we going to do about it?

@mcollina
Copy link
Member

@anonrig are you still blocking this?

Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

Biome is not ready to land yet. I think we should land this.

@targos
Copy link
Member Author

targos commented Apr 11, 2024

I'll update it asap

@targos targos added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Apr 13, 2024
@nodejs-github-bot

This comment was marked as outdated.

@targos targos added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 13, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 13, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 15, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 15, 2024
@nodejs-github-bot nodejs-github-bot merged commit ca7c8c6 into nodejs:main Apr 15, 2024
60 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in ca7c8c6

@targos targos deleted the stylistic branch April 15, 2024 15:10
aduh95 pushed a commit that referenced this pull request Apr 29, 2024
Refs: https://eslint.org/blog/2023/10/deprecating-formatting-rules/
PR-URL: #50714
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
Refs: https://eslint.org/blog/2023/10/deprecating-formatting-rules/
PR-URL: #50714
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
Refs: https://eslint.org/blog/2023/10/deprecating-formatting-rules/
PR-URL: #50714
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.