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: update examples of the final "callback" argument for fs.access() #20460

Closed
wants to merge 5 commits into from
Closed

doc: update examples of the final "callback" argument for fs.access() #20460

wants to merge 5 commits into from

Conversation

BeniCheni
Copy link
Contributor

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

Fixes: #17508

Per this following comment (as the origin of the aspiration to help out #17508), I've confirmed with the @ gireeshpunathil, and worked with the suggested content of #17578.

#17578 was never landed but has enough content in for a first timer to pick and progress.

Hope that the attempted fs.access snippets look okay in the PR. Happy to update according to review feedback. Thanks for reviewing in advance.

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels May 1, 2018
doc/api/fs.md Outdated
argument will be an `Error` object. The following example checks if the file
`/etc/passwd` can be read and written by the current process.
argument will be an `Error` object. The following examples check if
`package.json` exists, and/or if it could be written to.
Copy link
Member

Choose a reason for hiding this comment

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

Nits:

  • and/or -> and
  • could be written to -> is writable

doc/api/fs.md Outdated
fs.access('./package.json', fs.constants.W_OK, (err) => {
console.log(
err ?
'package.json could not be written to' :
Copy link
Member

Choose a reason for hiding this comment

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

Nit: could not be written to -> is not writable

doc/api/fs.md Outdated

// Check if package.json file could be read.
fs.access('./package.json', fs.constants.R_OK, (err) => {
console.log(err ? 'package.json could not be read' : 'package.json read');
Copy link
Member

Choose a reason for hiding this comment

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

Nit: could not be read -> is readable

Copy link
Contributor

Choose a reason for hiding this comment

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

is not readable / is readable?

Copy link
Member

Choose a reason for hiding this comment

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

@vsemozhetbyt Yes, I forgot the not. Should be:

Nit: could not be read -> is not readable

doc/api/fs.md Outdated
console.log(
err ?
'package.json could not be written to' :
'package.json could be written to'
Copy link
Member

Choose a reason for hiding this comment

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

Nit: could be written to -> is writable

doc/api/fs.md Outdated
});

// Check if `package.json` exists in the current directory and it could be
// written to.
Copy link
Member

Choose a reason for hiding this comment

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

Same with the comment: could be written to -> is writable

@Trott
Copy link
Member

Trott commented May 2, 2018

I wonder if the example code can be pared down a bit without losing any information. (I don't know if it can; I'm asking.)

@nodejs/documentation

@nodejs/fs

doc/api/fs.md Outdated
console.log(err ? 'package.json does not exist' : 'package.json exists');
});

// Check if package.json file could be read.
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt May 2, 2018

Choose a reason for hiding this comment

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

`package.json` for consistency?

@vsemozhetbyt
Copy link
Contributor

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite/645/

@BeniCheni
Copy link
Contributor Author

🙏 for 1st round of review @Trott & @vsemozhetbyt. I've read your feedback and updated the content to emphasize contextually more on the suggested "readable" & "writable" keywords in the latest updates. Please review again?

I also saw the "pared down a bit" comment, which I'll look out for any further suggested feedback to account for that potential update. (happy to update it to if any suggest arises on that topic)

doc/api/fs.md Outdated
// Check if `package.json` is writable.
fs.access('./package.json', fs.constants.W_OK, (err) => {
console.log(
err ?
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems this can be unwrapped now like in the block above, to spare some lines)

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented May 2, 2018

@BeniCheni @Trott Minimal changes I could think of (-11 lines without major refactoring):

Code:
const file = 'package.json';

// Check if the file exists in the current directory.
fs.access(file, fs.constants.F_OK, (err) => {
  console.log(`${file} ${err ? 'does not exist' : 'exists'}`);
});

// Check if the file is readable.
fs.access(file, fs.constants.R_OK, (err) => {
  console.log(`${file} ${err ? 'is not readable' : 'is readable'}`);
});

// Check if the file is writable.
fs.access(file, fs.constants.W_OK, (err) => {
  console.log(`${file} ${err ? 'is not writable' : 'is writable'}`);
});

// Check if the file exists in the current directory, and if it is writable.
fs.access(file, fs.constants.F_OK | fs.constants.W_OK, (err) => {
  if (err) {
    console.error(
      `${file} ${err.code === 'ENOENT' ? 'does not exist' : 'is read-only'}`);
  } else {
    console.log(`${file} exists, and it is writable`);
  }
});

@vsemozhetbyt
Copy link
Contributor

(And it seems // If there is an error, print the error in console and exit. can be deleted as self-evident; the proposal is updated.)

@BeniCheni
Copy link
Contributor Author

BeniCheni commented May 2, 2018

@vsemozhetbyt , nice refactoring suggestion. I worked with the suggested code to update the PR, as well as removed the not so useful // if there is an error, comment. This draft should look better?

doc/api/fs.md Outdated
const file = 'package.json';

// Check if the file exists in the current directory.
fs.access('./package.json', fs.constants.F_OK, (err) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can also replace './package.json' with just file here and in the next calls)

@BeniCheni
Copy link
Contributor Author

Sounds good, @vsemozhetbyt. Updated the ./package.json to just file variable in relevant places. 👁 the PR again?

@vsemozhetbyt vsemozhetbyt added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 2, 2018
@vsemozhetbyt
Copy link
Contributor

@Trott Can we land it in this variant?

@Trott
Copy link
Member

Trott commented May 3, 2018

@Trott Can we land it in this variant?

@vsemozhetbyt I have no objections.

@vsemozhetbyt
Copy link
Contributor

(Missed CI-lite link: https://ci.nodejs.org/job/node-test-pull-request-lite/650/)

vsemozhetbyt pushed a commit that referenced this pull request May 4, 2018
PR-URL: #20460
Fixes: #17508
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@vsemozhetbyt
Copy link
Contributor

Landed in b4ca3a4
Thank you!

MylesBorins pushed a commit that referenced this pull request May 4, 2018
PR-URL: #20460
Fixes: #17508
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@MylesBorins MylesBorins mentioned this pull request May 8, 2018
MylesBorins pushed a commit that referenced this pull request May 8, 2018
PR-URL: #20460
Fixes: #17508
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@BeniCheni BeniCheni deleted the fs-md-updt-dot-access branch August 24, 2018 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants