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

Jump to bottom

doc: some minor touch-ups (nitpicks) in the contributor docs #41966

Merged
merged 1 commit into from Feb 20, 2022
Merged

doc: some minor touch-ups (nitpicks) in the contributor docs #41966

merged 1 commit into from Feb 20, 2022

Conversation

@T-O-R-U-S
Copy link
Contributor

@T-O-R-U-S T-O-R-U-S commented Feb 14, 2022

Minor nitpicks, like changing things to adhere more closely to the style guide and some grammatical issues that I'd noticed.

There is also one stylistic change; please review and give me your opinion on it.
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Feb 14, 2022

Review requested:

@T-O-R-U-S T-O-R-U-S changed the title Some minor touch-ups (nitcpicks?) in the contributor docs. Doc: Some minor touch-ups (nitcpicks?) in the contributor docs. Feb 14, 2022
@Trott
Copy link
Member

@Trott Trott commented Feb 15, 2022

Might be good to put the Valgrind change into it's own commit and pull request. Additionally, you might consider opening a pull request to add a { yes: 'Valgrind' } entry to https://github.com/nodejs/remark-preset-lint-node/blob/8a44a025f54507118fc9369df23799001e3bbf22/index.js#L89-L105. That will (eventually) cause Node.js core linting to flag it any time someone uses "valgrind" instead of "Valgrind" in prose text. (It won't affect code examples or URLs or anything like that.)

@T-O-R-U-S
Copy link
Contributor Author

@T-O-R-U-S T-O-R-U-S commented Feb 15, 2022

I've created a PR to help with linting.

Working on splitting off the Valgrind change

@T-O-R-U-S T-O-R-U-S mentioned this pull request Feb 15, 2022
Closed
@T-O-R-U-S T-O-R-U-S mentioned this pull request Feb 15, 2022
Merged
@T-O-R-U-S
Copy link
Contributor Author

@T-O-R-U-S T-O-R-U-S commented Feb 18, 2022

Github automatically created a commit message for the above (which it didn't let me modify), so I'm adding it here;

Fixed git conflict with "Capitalize Valgrind in docs"; merged nodejs/node:master into `T-O-R-U-S/node:master'

mcollina
mcollina reviewed Feb 19, 2022
View changes
doc/contributing/investigating-native-memory-leaks.md Outdated
@@ -423,6 +423,7 @@ git checkout v12.14.1
[building a debug build](https://github.com/nodejs/node/blob/v12.14.1/BUILDING.md#building-a-debug-build)).
For example, on \*nix:
Copy link
Member

@mcollina mcollina Feb 19, 2022

Choose a reason for hiding this comment

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

could you remove this space?

Copy link
Contributor Author

@T-O-R-U-S T-O-R-U-S Feb 19, 2022

Choose a reason for hiding this comment

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

Done!

aduh95
aduh95 approved these changes Feb 19, 2022
View changes
DerekNonGeneric
DerekNonGeneric approved these changes Feb 19, 2022
View changes
Copy link
Contributor

@DerekNonGeneric DerekNonGeneric left a comment •
edited

Thanks, just got around to watching the second half of the mini-summit. These suggestions seem worth applying (amongst others; see below).

Also would like it if the majority of each line's max-length (80) gets used now that things are a bit shifted elsewhere.

Really, really sorry to bother you, @T-O-R-U-S!

doc/contributing/maintaining-types-for-nodejs.md Outdated Show resolved Hide resolved
doc/contributing/maintaining-types-for-nodejs.md Outdated Show resolved Hide resolved
doc/contributing/maintaining-types-for-nodejs.md Outdated Show resolved Hide resolved
doc/contributing/maintaining-types-for-nodejs.md Outdated Show resolved Hide resolved
doc/contributing/maintaining-types-for-nodejs.md Outdated Show resolved Hide resolved
doc/contributing/maintaining-types-for-nodejs.md Outdated Show resolved Hide resolved
@DerekNonGeneric DerekNonGeneric changed the title Doc: Some minor touch-ups (nitcpicks?) in the contributor docs. doc: some minor touch-ups (nitpicks) in the contributor docs Feb 19, 2022
@T-O-R-U-S
Copy link
Contributor Author

@T-O-R-U-S T-O-R-U-S commented Feb 20, 2022

Thank you @DerekNonGeneric!

mcollina
mcollina approved these changes Feb 20, 2022
View changes
Copy link
Member

@mcollina mcollina left a comment

lgtm

Trott
Trott approved these changes Feb 20, 2022
View changes
@T-O-R-U-S @Trott
doc: make minor fixes to contributing guides
013f20c 
PR-URL: nodejs#41966
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott
Copy link
Member

@Trott Trott commented Feb 20, 2022

Landed in 013f20c

@Trott Trott merged commit 013f20c into nodejs:master Feb 20, 2022
11 checks passed
bengl added a commit to bengl/node that referenced this issue Feb 21, 2022
@T-O-R-U-S @bengl
doc: make minor fixes to contributing guides
c3fab79 
PR-URL: nodejs#41966
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
bengl added a commit to bengl/node that referenced this issue Feb 21, 2022
@T-O-R-U-S @bengl
doc: make minor fixes to contributing guides
314e910 
PR-URL: nodejs#41966
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
bengl added a commit that referenced this issue Feb 21, 2022
@T-O-R-U-S @bengl
doc: make minor fixes to contributing guides
e279e9c 
PR-URL: #41966
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@bengl bengl mentioned this pull request Feb 21, 2022
Merged
bengl added a commit that referenced this issue Feb 21, 2022
@T-O-R-U-S @bengl
doc: make minor fixes to contributing guides
9ab2e21 
PR-URL: #41966
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
bengl added a commit that referenced this issue Feb 22, 2022
@T-O-R-U-S @bengl
doc: make minor fixes to contributing guides
9c618ad 
PR-URL: #41966
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina

@jasnell jasnell

@Trott Trott

@aduh95 aduh95

@DerekNonGeneric DerekNonGeneric

@RaisinTen RaisinTen

Assignees
No one assigned
Labels
doc
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet
8 participants
@T-O-R-U-S @nodejs-github-bot @Trott @mcollina @jasnell @aduh95 @DerekNonGeneric @RaisinTen