Skip to content

Conversation

@lance
Copy link
Member

@lancelance commented Jun 2, 2016

Checklist
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

doc, fs

Description of change

Fixes: #7110

This is maybe more verbose than needed, since the same information is
repeated several times. An alternative, maybe a single short sentence at
the beginning is better. E.g.

Tests a user's permissions for the file or directory specified by
path. All modes work for either files or directories. mode is...

Fixes: #7110 This is maybe more verbose than needed, since the same information is repeated several times. An alternative, maybe a single short sentence at the beginning is better. E.g. > Tests a user's permissions for the file or directory specified by > path. All modes work for either files or directories. `mode` is...
@nodejs-github-botnodejs-github-bot added the doc Issues and PRs related to the documentations. label Jun 2, 2016
@cjihrig
Copy link
Contributor

Could you just change the first sentence to:

Tests a user's permissions for the file or directory specified by path.

As you already did. Then, for F_OK and friends, just change "File" to path.

No need to be so verbose about file vs. directory. They are all just paths.
@lance
Copy link
MemberAuthor

lance commented Jun 2, 2016

@cjihrig simplified... Just noticed that my editor trimmed some trailing whitespace. I assume this is not a problem.

doc/api/fs.md Outdated
possible to create a mask consisting of the bitwise OR of two or more values.

-`fs.constants.F_OK` - File is visible to the calling process. This is useful
-`fs.constants.F_OK` - Path is visible to the calling process. This is useful
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I meant use path, formatted as code to reference the argument.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I should have realized... Fixed

@cjihrig
Copy link
Contributor

LGTM

@mscdexmscdex added the fs Issues and PRs related to the fs subsystem / file system. label Jun 2, 2016
doc/api/fs.md Outdated
following constants define the possible values of `mode`. It is possible to
create a mask consisting of the bitwise OR of two or more values.
Tests a user's permissions for the file or directory specified by `path`.
`mode` is an optional integer that specifies the accessibility checks to be
Copy link
Member

@jasnelljasnellJun 2, 2016

Choose a reason for hiding this comment

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

s/mode is/The mode argument is

@jasnell
Copy link
Member

Small nit, otherwise LGTM

@lance
Copy link
MemberAuthor

lance commented Jun 6, 2016

@jasnell@cjihrig nits addressed

@cjihrig
Copy link
Contributor

Still LGTM

jasnell pushed a commit that referenced this pull request Jun 6, 2016
This is maybe more verbose than needed, since the same information is repeated several times. An alternative, maybe a single short sentence at the beginning is better. E.g. Fixes: #7110 PR-URL: #7113 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

Landed in d976d66

@evanlucas
Copy link
Contributor

This is not landing cleanly on the v6.x branch. @lance would you be interested in opening an additional PR targeting the v6.x branch?

@lance
Copy link
MemberAuthor

@evanlucas will do. A question. It's not clear to me if @jasnell's dcccbfd#diff-acabf706a8aa070a8796e3573f7e4678 is intended to land in v6.x. If so, the docs should refer to fs.constants.R_OK etc. If not, fs.R_OK. Since there is this #6534 (comment), I'm going to assume that I should leave as fs.R_OK on v6.x branch for the time being, since that should work whether @jasnell's commit lands in the branch or not.

@lancelance mentioned this pull request Jun 16, 2016
3 tasks
@lance
Copy link
MemberAuthor

@evanlucas
Copy link
Contributor

@lance yea, the linked pr is semver-minor so won't be landing until the next semver-minor bump

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docIssues and PRs related to the documentations.fsIssues and PRs related to the fs subsystem / file system.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@lance@cjihrig@jasnell@evanlucas@mscdex@MylesBorins@nodejs-github-bot