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

Jump to bottom

doc,lib,test: capitalize comment sentences #24996

Closed
wants to merge 1 commit into from
Closed

doc,lib,test: capitalize comment sentences #24996

wants to merge 1 commit into from

Conversation

BridgeAR
Copy link
Member

This expands the eslint capitalize comment rule to be active for comments
above 50 characters (instead of 62) and ignores starting words which contain # or [.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@BridgeAR build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1914/pipeline

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Dec 12, 2018
@Trott
Copy link
Member

Trott commented Dec 12, 2018

Not an objection, but an observation/concern: I'm not sure how I feel about codifying the backticks-to-surround-code in comments. That's a markdown typography convention, not a widely-understood convention for plain-text comments. (We understand it because we're editing markdown all the time.)

@Trott
Copy link
Member

Trott commented Dec 12, 2018

With all this churn, and the brittle magic number for line length to ignore, I wonder if what we really should do is stop leaving nits for capitalizing comments. Make peace with the fact that some comments will be capitalized and others won't. This does not significantly improve our code.

Still not feeling an objection, but wondering if that's where I'm headed and curious if others feel the same or not....

Trott
Trott reviewed Dec 12, 2018
View reviewed changes
benchmark/net/tcp-raw-c2s.js Outdated Show resolved Hide resolved
Trott
Trott reviewed Dec 12, 2018
View reviewed changes
benchmark/net/tcp-raw-s2c.js Outdated Show resolved Hide resolved
@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 13, 2018 •
edited
Loading

@Trott

I'm not sure how I feel about codifying the backticks-to-surround-code in comments

We use a lot of markdown and having them as code comments should be natural for anyone working on the source. Having them in the docs might be less ideal. Do you have a good alternative at hand?

The brittle magic number for line length to ignore

I was not sure if it's to early to open this PR or not but I would like to reduce that magic number from time to time (each couple minor releases) so that at some point we don't have a character limit at all. It was just a way to limit the churn a bit.

I wonder if what we really should do is stop leaving nits for capitalizing comments. [...] This does not significantly improve our code.

While I agree that it's not a huge improvement, it's just a one time task as most of our small improvements. As soon as the rule is fully introduces we do not have to worry about any of it again :) Since we are human, it's difficult to stop leaving the nits for such things.

@Trott
Copy link
Member

Trott commented Dec 13, 2018

Do you have a good alternative at hand?

@BridgeAR No. I think I'm OK with it. It's not ideal IMO but I also don't have a better idea, so I shouldn't block progress. If I ever think of a better idea, I can open a PR changing it to something else. :-D

I was not sure if it's to early to open this PR or not but I would like to reduce that magic number from time to time

Worth a shot! Thanks for explaining that.

Since we are human, it's difficult to stop leaving the nits for such things.

I certainly can't dispute that. Very true.

ryzokuken
ryzokuken approved these changes Dec 13, 2018
View reviewed changes
Copy link
Contributor

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

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

+1 for the improved rule.

@BridgeAR
Copy link
Member Author

Rebased due to conflicts.

Lite CI https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/1925/

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 13, 2018
@BridgeAR
Copy link
Member Author

@nodejs/linting @nodejs/documentation PTAL

@Trott
Copy link
Member

Trott commented Dec 15, 2018 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/19560/ ✔️

@BridgeAR
Copy link
Member Author

It would be great to get another review.

@BridgeAR
doc,lib,test: capitalize comment sentences
eccc1db 
This activates the eslint capitalize comment rule for comments
above 50 characters.
@BridgeAR
Copy link
Member Author

Rebased due to conflicts.

CI before landing: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/1980/

BridgeAR added a commit to BridgeAR/node that referenced this pull request Dec 17, 2018
@BridgeAR
doc,lib,test: capitalize comment sentences
50dd555 
This activates the eslint capitalize comment rule for comments
above 50 characters.

PR-URL: nodejs#24996
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR
Copy link
Member Author

Landed in 50dd555

@BridgeAR BridgeAR closed this Dec 17, 2018
MylesBorins pushed a commit that referenced this pull request Dec 25, 2018
@BridgeAR @MylesBorins
doc,lib,test: capitalize comment sentences
2516e9c 
This activates the eslint capitalize comment rule for comments
above 50 characters.

PR-URL: #24996
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 25, 2018
Merged
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@BridgeAR @refack
doc,lib,test: capitalize comment sentences
0328eeb 
This activates the eslint capitalize comment rule for comments
above 50 characters.

PR-URL: nodejs#24996
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR deleted the more-eslint-stuff branch January 20, 2020 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos left review comments

@Trott Trott Trott left review comments

@ryzokuken ryzokuken ryzokuken approved these changes

@addaleax addaleax addaleax approved these changes

@thefourtheye thefourtheye thefourtheye approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis Awaiting requested review from bnoordhuis

@rubys rubys Awaiting requested review from rubys

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@BridgeAR @nodejs-github-bot @Trott @jasnell @thefourtheye @addaleax @targos @ryzokuken @BethGriggs