tools: update capital-comments eslint rule #27675
Conversation
nodejs-github-bot
added
the
lib / src
|
These changes seem needless, and have caused us (NodeSource) plenty of unnecessary rebase grief (for our Node runtime, N|Solid). Clearly, that's what we signed up for but it is certainly very annoying when the changes have little functional or even human documentation purpose aside from minor grammatical correctness. |
This makes sure comments from 10 characters on are validated instead of 20 characters as lower bound. The regular expression was also updated to ignore variable declarations which might happen while commenting out some code during development.
BridgeAR
force-pushed
the
more-capital-comments
branch
from
b1f4118 to
69a0035
Compare
|
@Fishrock123 I did indeed not think about third parties in this case. Do you cherry-pick individual commits or does it hurt backporting commits? That aside: I changed the PR to only reduce the length check to 18 instead of 10 and now there are only 35 lines of code impacted. I can also keep it at 20 if you still think that might still cause trouble but I would definitely like to add the other changes (that relax the rule instead of making it stricter). That way some comments won't complain anymore (like commenting out code). |
|
@BridgeAR Regardless of forks of Node’s codebase, and regardless of this specific PR, these types of changes generally just create a lot of work with almost nothing gained from them. I think it makes far more sense to keep doing these as drive-by changes when neighbouring lines are being modified. And in terms of backporting, I’ve just had multiple unnecessary conflicts because of changed comment casing with backporting brotli to v10.x. I’d really prefer to just not do this. |
|
@addaleax we currently work a bit on our backporting strategy in the release working group to make backporting easier (we try to backport as much as possible in the correct order and use compatibility patches for semver-major changes). That should ideally remove most conflicts, no matter of a change like this. I updated the PR to only relax the rule (so it'll complain about less comments). That should be pretty uncontroversial? |
BridgeAR
changed the title
| @@ -66,7 +66,8 @@ module.exports = { | |||
| line: { | |||
| // Ignore all lines that have less characters than 20 and all lines that | |||
| // start with something that looks like a variable name or code. | |||
| ignorePattern: '.{0,20}$|[a-z]+ ?[0-9A-Z_.(/=:[#-]|std', | |||
| // eslint-disable-next-line max-len | |||
| ignorePattern: '.{0,20}$|[a-z]+ ?[0-9A-Z_.(/=:[#-]|std|http|ssh|ftp|(let|var|const) [a-z_A-Z0-9]+ =|[b-z] |[a-z]*[0-9].* ', | |||
There was a problem hiding this comment.
This regexp is so long and complex, might it just be better to not have the rule at all?
From my perspective, the only reason to have a rule like this is so that people don't get nits about it in code reviews. If a tool can point it out, let's save everyone the work. But back-porting headaches and discussion about this rule and having to debug that regexp should it ever come to that...all of that time/energy may exceed the benefit of not having to leave/discuss those nits. Maybe we can better invest the time/energy to get consensus that we really don't actually care about capitalization, punctuation, and even spelling in code comments as long as the comments are clear first and foremost, and relatively professional and welcoming beyond that. (Honestly, I think @bnoordhuis was the only one who left nits about comment capitalization more than once or twice, and they didn't exactly do it very often and generally oppose churn anyway so might prefer to not land this too?)
There was a problem hiding this comment.
I personally like the rule. It feels wrong to me to have lower cased comments but that might just be me. It's indeed not easy to find all exceptions but it worked relatively well (the code base is huge and we have a bunch of different comments while this expression seems to handle pretty much all cases that should not be updated well).
The only reason I wanted to update this again is that my linter would auto correct e.g. var to Var as soon as I comment out some code. While being on it I also added some further exceptions.
There was a problem hiding this comment.
(Just to be clear in case my comment above seems blocking: I don't oppose this change.)
BridgeAR
added
the
author ready
The regular expression is updated to ignore variable declarations which might happen while commenting out some code during development and adds a couple more exceptions to prevent false negatives. PR-URL: nodejs#27675 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]>
|
Landed in 5b26609 🎉 |
The regular expression is updated to ignore variable declarations which might happen while commenting out some code during development and adds a couple more exceptions to prevent false negatives. PR-URL: #27675 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]>
Please have a look at the commit messages for details.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes