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.

Sign up for GitHub

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: update capital-comments eslint rule #27675

Closed
wants to merge 4 commits into from
Closed

tools: update capital-comments eslint rule #27675

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
958ba5e
doc: update comments to use capital letters
BridgeAR May 13, 2019
69a0035
tools: update capitalized-comments eslint rule
BridgeAR May 13, 2019
1130ad3
fixup! tools: update capitalized-comments eslint rule
BridgeAR May 13, 2019
96c48d5
Revert "doc: update comments to use capital letters"
BridgeAR May 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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].* ',
Copy link
Member

Choose a reason for hiding this comment

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

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?)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@Trott Trott May 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

(Just to be clear in case my comment above seems blocking: I don't oppose this change.)

ignoreInlineComments: true,
ignoreConsecutiveComments: true,
},
Expand Down