-
Notifications
You must be signed in to change notification settings - Fork 29.1k
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: remove readability/fn_size rule #54663
Conversation
FWIW it's already appearing in PRs adding to the CLI flags. |
ping @nodejs/build @nodejs/linting |
I can attest. Got it in #53060 |
@@ -325,7 +325,6 @@ | |||
'readability/casting', | |||
'readability/check', | |||
'readability/constructors', | |||
'readability/fn_size', |
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.
IMO this should be commented out with a note as to why?
Landed in 3d954dc |
Still getting the error after rebasing in #53060. It's blocking that PR from landing. @RafaelGSS May something have been missed by any chance? |
Weird. Does it fail locally? |
@RafaelGSS Yup Edit: currently running a |
See: #54744 |
Nice! Thanks for the quick response, that should do it👍🏼 |
PR-URL: #54663 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: #54663 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Due to #53927 I had to update the v22.x-staging branch to include a
// NOLINT(readability/fn_size)
https://github.com/nodejs/node/blob/v22.x-staging/src/node_options.cc#L890I'm questioning whether having this rule is really necessary or beneficial. This lint didn't fail on
main
(yet) because we have more code on this specific function in v22.x-staging. So, this will likely appear in a PR soon, and I think that dropping this rule instead of adding aNOLINT
comment is a much better option.