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: Clarify fs.access works on directories too. #7113

Closed
wants to merge 4 commits into from
Closed

doc: Clarify fs.access works on directories too. #7113

wants to merge 4 commits into from

Conversation

lance
Copy link
Member

@lance lance 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...

@lance
doc: Clarify fs.access works on directories too.
776cbfb 
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-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jun 2, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Jun 2, 2016

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.

@lance
doc,fs: Simplify fs.access documentation.
8f363bf 
No need to be so verbose about file vs. directory. They are all just paths.
@lance
Copy link
Member Author

lance commented Jun 2, 2016 •
edited
Loading

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

cjihrig
cjihrig reviewed Jun 2, 2016
View reviewed changes
doc/api/fs.md

- `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
Member Author

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

cjihrig commented Jun 2, 2016

LGTM

@mscdex mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Jun 2, 2016
jasnell
jasnell reviewed Jun 2, 2016
View reviewed changes
doc/api/fs.md
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

@jasnell jasnell Jun 2, 2016 •
edited
Loading

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

jasnell commented Jun 2, 2016

Small nit, otherwise LGTM

@lance
Copy link
Member Author

lance commented Jun 6, 2016

@jasnell @cjihrig nits addressed

@cjihrig
Copy link
Contributor

cjihrig commented Jun 6, 2016

Still LGTM

jasnell pushed a commit that referenced this pull request Jun 6, 2016
@lance @jasnell
doc: clarify fs.access works on directories too.
d976d66 
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 <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Jun 6, 2016

Landed in d976d66

@jasnell jasnell closed this Jun 6, 2016
@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
Member Author

lance commented Jun 16, 2016

@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.

@lance lance mentioned this pull request Jun 16, 2016
Closed
3 tasks
@lance
Copy link
Member Author

lance commented Jun 16, 2016

@evanlucas @jasnell #7321

@evanlucas
Copy link
Contributor

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

@MylesBorins MylesBorins mentioned this pull request Jul 11, 2016
Closed
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@lance @cjihrig @jasnell @evanlucas @mscdex @MylesBorins @nodejs-github-bot