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

doc: add an example code #35625

Merged
merged 1 commit into from
Oct 15, 2020
Merged

doc: add an example code #35625

merged 1 commit into from
Oct 15, 2020

Conversation

PoojaDurgad
Copy link
Contributor

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 nodejs-github-bot added doc Issues and PRs related to the documentations. process Issues and PRs related to the process subsystem. labels Oct 13, 2020
@@ -1342,6 +1342,12 @@ The `process.getgroups()` method returns an array with the supplementary group
IDs. POSIX leaves it unspecified if the effective group ID is included but
Node.js ensures it always is.

```js
if (process.getgroups) {
console.log(`Current groups: ${process.getgroups()}`);
Copy link
Member

@Trott Trott Oct 13, 2020

Choose a reason for hiding this comment

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

Suggested change
console.log(`Current groups: ${process.getgroups()}`);
console.log(process.getgroups()); // [ 16, 21, 297 ]

Embedding it in a string might make it less clear what the output is in this case. Raw array output might be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Trott - I understand your point and thanks for the review. I have followed your suggestion. Please have a look.

PR-URL: nodejs#35625
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott Trott merged commit f59d4e0 into nodejs:master Oct 15, 2020
@Trott
Copy link
Member

Trott commented Oct 15, 2020

Landed in f59d4e0

@PoojaDurgad I usually don't bother contributors with this detail, but because you are opening a lot of pull requests: Usually, only the first commit needs to be formatted according to our guidelines. For subsequent commits that are small changes to your pull requests (such as in this one where you had a second commit that removed extraneous whitespace and a third commit that implemented a small suggestion from a reviewer), it helps if you use git commit --fixup HEAD rather than writing a commit message. This will both let someone who lands the commit manually use --autosquash and it also means that your pull requests can be correctly landed by the bot we use (because it will run --autosquash if it can).

@PoojaDurgad
Copy link
Contributor Author

@Trott - Thanks for the suggestion. I will follow up.

BethGriggs pushed a commit that referenced this pull request Dec 8, 2020
PR-URL: #35625
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
PR-URL: nodejs#35625
Reviewed-By: Luigi Pinca <luigipinca@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
Labels
doc Issues and PRs related to the documentations. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants