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.

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

unneded "." in pull-requests.md file #45502

Closed
nikitakoselev opened this issue Nov 17, 2022 · 4 comments · Fixed by #45519
Closed

unneded "." in pull-requests.md file #45502

nikitakoselev opened this issue Nov 17, 2022 · 4 comments · Fixed by #45519

Comments

@nikitakoselev
Copy link

Version

1af63a9

Platform

github

Subsystem

node/doc/contributing/pull-requests.md

What steps will reproduce the bug?

Intro:
Open the main branch, commit 1af63a9 of nodejs/node
File pull-requests.md

Bug:
There is an unneeded "." in the end of the "tests within the test directory." line.

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior?

No response

What do you see instead?

image

Additional information

I suggest this bug is marked with "good first time issue" so a new joiner could fix it.

@aduh95
Copy link
Contributor

aduh95 commented Nov 18, 2022

It's unclear to me if all items should end with a colon, or if no items should end with a colon. I agree that the lack of consistency is odd though.

Refs: https://learn.microsoft.com/en-us/style-guide/scannable-content/lists#punctuation

/cc @nodejs/documentation @Trott

@anonrig
Copy link
Member

anonrig commented Nov 18, 2022

I found a particular article mentioning this:

Use a period after bullet list that completes the opening stem sentence that introduces it. Don't use a period after bullet lists that are not complete sentences or do not complete the opening stem sentence.

@Trott
Copy link
Member

Trott commented Nov 19, 2022

It's unclear to me if all items should end with a colon, or if no items should end with a colon. I agree that the lack of consistency is odd though.

Refs: https://learn.microsoft.com/en-us/style-guide/scannable-content/lists#punctuation

/cc @nodejs/documentation @Trott

In this case, I would propose one of the following:

  1. Remove the list content entirely. It's not needed.

  2. Convert the list content into a sentence.

Most pull requests in Node.js include changes to the C++ code
in the `src` directory, the JavaScript code in the `lib` directory,
documentation in the `doc/api` directory, and/or test in the `test` directory.
  1. Rewrite the sentence fragment introducing the list.
Pull requests in Node.js typically involve changes to
one or more of a few places in the code base.

* C/C++ code contained in the `src` directory
* JavaScript code contained in the `lib` directory
* Documentation in `doc/api`
* Tests within the `test` directory

@Trott
Copy link
Member

Trott commented Nov 19, 2022

I did option 3 in #45519.

nodejs-github-bot pushed a commit that referenced this issue Nov 19, 2022
Fixes: #45502
PR-URL: #45519
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
ruyadorno pushed a commit that referenced this issue Nov 21, 2022
Fixes: #45502
PR-URL: #45519
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
marco-ippolito pushed a commit to marco-ippolito/node that referenced this issue Nov 23, 2022
Fixes: nodejs#45502
PR-URL: nodejs#45519
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
danielleadams pushed a commit that referenced this issue Dec 30, 2022
Fixes: #45502
PR-URL: #45519
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
danielleadams pushed a commit that referenced this issue Dec 30, 2022
Fixes: #45502
PR-URL: #45519
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
danielleadams pushed a commit that referenced this issue Jan 3, 2023
Fixes: #45502
PR-URL: #45519
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
danielleadams pushed a commit that referenced this issue Jan 4, 2023
Fixes: #45502
PR-URL: #45519
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
danielleadams pushed a commit that referenced this issue Jan 5, 2023
Fixes: #45502
PR-URL: #45519
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants