FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

benchmark,doc,lib,test: capitalize comments by BridgeAR · Pull Request #26223 · nodejs/node · GitHub

/ node Public

benchmark,doc,lib,test: capitalize comments - #26223

Closed
BridgeAR wants to merge 3 commits into
nodejs:masterfrom
BridgeAR:capitalize-more
ConversationCommits (3)ChecksFiles changed
Closed

benchmark,doc,lib,test: capitalize comments#26223
BridgeAR wants to merge 3 commits into
nodejs:masterfrom
BridgeAR:capitalize-more

Conversation

BridgeAR commented Feb 20, 2019

Copy link
Copy Markdown
Member

This updates a lot of comments.

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

benchmark,doc,lib,test: capitalize comments
7239ca1 
This updates a lot of comments.
BridgeAR requested review from Trott and ryzokuken February 20, 2019 22:00

nodejs-github-bot commented Feb 20, 2019

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Feb 20, 2019
Trott approved these changes Feb 20, 2019
View reviewed changes

Trott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

RSLGTM, but are you willing to backport to the four current release branches (11, 10, 8, 6) so as to avoid endless merge conflicts going forward?

(I guess if churn is a concern, we can always try 45 or 49 instead of 40?)

BridgeAR commented Feb 20, 2019

Copy link
Copy Markdown
Member Author

@Trott yes, I am willing to do that. So far it seemed like the introduction of the rule itself did not cause any pain in backporting. It's probably because it's mostly just a single line in a comment and this can mostly be resolved automatically.

vsemozhetbyt added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 21, 2019
vsemozhetbyt approved these changes Feb 21, 2019
View reviewed changes

vsemozhetbyt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

With in-passing nits, feel free to ignore or to address in a fixup commit/other PR.

Comment thread test/parallel/test-stream-readable-event.js Outdated
Comment thread test/parallel/test-buffer-tostring-range.js Outdated
vsemozhetbyt and others added 2 commits February 21, 2019 12:54
Fixup: typo in comment
76b0553 
Co-Authored-By: BridgeAR <ruben@bridgewater.de>
Fixup: typo in comment
41cbcd0 
Co-Authored-By: BridgeAR <ruben@bridgewater.de>
antsmartian approved these changes Feb 21, 2019
View reviewed changes

antsmartian left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks @BridgeAR 👍 👍

BridgeAR commented Feb 22, 2019

Copy link
Copy Markdown
Member Author

Lite-CI https://ci.nodejs.org/job/node-test-commit-lite/1229/

BridgeAR added a commit to BridgeAR/node that referenced this pull request Feb 28, 2019
benchmark,doc,lib,test: capitalize comments
9edce1e 
This updates a lot of comments.

PR-URL: nodejs#26223
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>

BridgeAR commented Feb 28, 2019

Copy link
Copy Markdown
Member Author

Landed in 9edce1e

BridgeAR closed this Feb 28, 2019

addaleax commented Mar 1, 2019

Copy link
Copy Markdown
Member

This needs a manual backport for v11.x, which should probably happen soon, given that this changed 200 files.

It might also make sense to not land the bits that change the linter rules for the release branches, because that can get in the way of backporting this? (Maybe it could have been a separate commit? I know other people do it that way when modifying/adding/enabling linter rules.)

BridgeAR added a commit that referenced this pull request Mar 4, 2019
benchmark,doc,lib,test: capitalize comments
d66cb4a 
This updates a lot of comments.

PR-URL: #26223
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>

BridgeAR commented Mar 4, 2019

Copy link
Copy Markdown
Member Author

I backported this directly to the staging branch.

@addaleax I'll separate the PRs in the future. That is indeed a good way of dealing with that. In this case I just ran make lint-js-fix after resolving the conflict (one line had to be fixed) and adding that to other cases (two other lines had to be changed).

BridgeAR mentioned this pull request Mar 4, 2019
Merged
BridgeAR deleted the capitalize-more branch January 20, 2020 11:53
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
Trott Trott approved these changes
ryzokuken Awaiting requested review from ryzokuken
+2 more reviewers
antsmartian antsmartian approved these changes
vsemozhetbyt vsemozhetbyt approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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.

7 participants

Back | FazBrowse Home | New Git URL