Skip to content

Conversation

@Trott
Copy link
Member

Remove/add periods as appropriate in bulleted lists in BUILDING.md.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-botnodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. labels Aug 20, 2020
@jasnelljasnell added the fast-track PRs that do not need to wait for 48 hours to land. label Aug 20, 2020
@jasnell
Copy link
Member

Shouldn't be any reason for this PR to wait to land once it has appropriate review. Fast-track? Please 👍🏻

@rickyesrickyes added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 20, 2020
BUILDING.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

This bullet is also a single sentence that ends in a period.

Copy link
MemberAuthor

@TrottTrottAug 20, 2020

Choose a reason for hiding this comment

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

Ideally, everything in the list should be the same structure and also grammatically correct. Unfortunately, this one is neither. It starts with a phrase (like the previous items in the list) and then has a full sentence bolted onto it with a comma-splice.

I'm inclined to leave it for another round of fixing later. This PR is all tiny changes that I think should be uncontroversial. Once I start rewording things, moving stuff around, and adding parentheses or whatever, there may be more bike-shedding (which is fine, but I'd like to keep that separate from the more obvious stuff that can/should land quickly).

Trott added a commit that referenced this pull request Aug 20, 2020
Remove/add periods as appropriate in bulleted lists in BUILDING.md. PR-URL: #34849 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ricky Zhou <[email protected]>
@Trott
Copy link
MemberAuthor

Landed in 9fd71a9

@TrottTrott closed this Aug 20, 2020
@TrottTrott deleted the list-punctuation branch August 20, 2020 17:30
@richardlau
Copy link
Member

@Trott As an aside, this PR is showing 0 commits and 0 file changed, probably because it was closed before GitHub could update status. There's a note about it at the end of https://github.com/nodejs/node/blob/master/doc/guides/collaborator-guide.md#technical-howto.

@Trott
Copy link
MemberAuthor

@Trott As an aside, this PR is showing 0 commits and 0 file changed, probably because it was closed before GitHub could update status. There's a note about it at the end of https://github.com/nodejs/node/blob/master/doc/guides/collaborator-guide.md#technical-howto.

Ah, thanks for calling my attention to that! I always push to master before pushing to my branch. Hopefully my muscle-memory won't get in the way of me reversing that process. (Maybe it's time for me to write a script.)

BethGriggs pushed a commit that referenced this pull request Aug 24, 2020
Remove/add periods as appropriate in bulleted lists in BUILDING.md. PR-URL: #34849 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ricky Zhou <[email protected]>
@danielleadamsdanielleadams mentioned this pull request Aug 25, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.buildIssues and PRs related to build files or the CI.docIssues and PRs related to the documentations.fast-trackPRs that do not need to wait for 48 hours to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@Trott@jasnell@richardlau@rickyes@nodejs-github-bot