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.

Sign up for GitHub

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: update umask for clarity #14170

Closed
wants to merge 2 commits into from
Closed

doc: update umask for clarity #14170

wants to merge 2 commits into from

Conversation

jsumners
Copy link
Contributor

@jsumners jsumners commented Jul 11, 2017 •
edited by gibfahn
Loading

This PR clarifies the documentation for process.umask. It resolves issue #14169.

Checklist
Affected core subsystem(s)

doc

Fixes: #14169

@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 Jul 11, 2017
cjihrig
cjihrig approved these changes Jul 11, 2017
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM with a nit.

doc/api/process.md Outdated
creation mask. Child processes inherit the mask from the parent process. The old
mask is return if the `mask` argument is given, otherwise returns the current
mask.
creation mask. Child processes inherit the mask from the parent process. Invoked without an argument, the current mask is returned, otherwise the umask is set to the argument value and the previous mask is returned.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please wrap lines at 80 characters.

@gibfahn
Copy link
Member

gibfahn commented Jul 11, 2017 •
edited
Loading

Could you add a Fixes line to the first commit?

doc: update umask for clarity

Fixes: https://github.com/nodejs/node/issues/14169

Fixes: #14169

@jsumners
Copy link
Contributor Author

I don't know how to do that.

@gibfahn
Copy link
Member

gibfahn commented Jul 12, 2017

I don't know how to do that.

no problem, it can be done by whoever lands the PR.

Also @refack just pointed out to me that our PR metadata generator picks it up from the first comment message, so I just put in there instead.

gireeshpunathil
gireeshpunathil approved these changes Jul 12, 2017
View reviewed changes
Copy link
Member

@gireeshpunathil gireeshpunathil left a comment

Choose a reason for hiding this comment

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

FWIW, this polymorphic return style appeared in as early as 1970's in the CAS instructions.

@jsumners
Copy link
Contributor Author

@gireeshpunathil the issue wasn't about the functionality of the function. It was the description of that functionality.

jasnell pushed a commit that referenced this pull request Jul 14, 2017
@jsumners @jasnell
doc: update umask for clarity
dce2f06 
PR-URL: #14170
Fixes: #14169
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Jul 14, 2017

Landed in dce12f06

@jasnell jasnell closed this Jul 14, 2017
@addaleax addaleax mentioned this pull request Jul 18, 2017
Merged
addaleax pushed a commit that referenced this pull request Jul 18, 2017
@jsumners @addaleax
doc: update umask for clarity
e91a7a4 
PR-URL: #14170
Fixes: #14169
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
@jsumners @Fishrock123
doc: update umask for clarity
b344687 
PR-URL: #14170
Fixes: #14169
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
@jsumners @MylesBorins
doc: update umask for clarity
7bf6f24 
PR-URL: #14170
Fixes: #14169
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
@jsumners @MylesBorins
doc: update umask for clarity
98466c5 
PR-URL: #14170
Fixes: #14169
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 5, 2017
@jsumners @MylesBorins
doc: update umask for clarity
b0286ac 
PR-URL: #14170
Fixes: #14169
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This was referenced Sep 5, 2017
Closed
Closed
Closed
Closed
@beevelop beevelop mentioned this pull request Sep 6, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibfahn gibfahn gibfahn approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. process Issues and PRs related to the process subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

doc: process.umask doc unclear
9 participants
@jsumners @gibfahn @jasnell @refack @lpinca @cjihrig @gireeshpunathil @MylesBorins @nodejs-github-bot