Skip to content

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
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
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
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
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
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
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
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
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 <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@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 <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@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 <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@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