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: add example for file existence with fs.stat #8585

Closed
wants to merge 3 commits into from
Closed

doc: add example for file existence with fs.stat #8585

wants to merge 3 commits into from

Conversation

connium
Copy link

@connium connium commented Sep 17, 2016

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

doc

Description of change

Add an example on how to test if a file exists with fs.stat. Also add
a link to the Common System Errors.

Fixed with help of @dschnei @narf101010

Fixes: #6752

Add an example on how to test if a file exists with fs.stat. Also add
a link to the Common System Errors.

Fixes: https://github.com/nodejs/issues/6752
@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 Sep 17, 2016
@imyller
Copy link
Member

imyller commented Sep 17, 2016

If there is need to document an example of fs.stat usage for checking file existince, I have few reservations about this implementation:

  • Checking for ENOENT alone might not be enough to indicate file (non)existence (which is more complex issue than one might initially think). Considering all the imaginable filesystem, file consistency and platform variants.
  • fs.Stats.isFile() and fs.Stats.isDirectory() methods could be more reliable cross-platform ways to determine file or directory existence than Common System Error code.

More collaborator feedback needed here.

@bnoordhuis
Copy link
Member

If you just want to check that a file exists, fs.access() is a better, more efficient solution.

If you want to manipulate the file, stat-then-open (or stat-then-move, etc.) is subject to TOCTOU race conditions. The right way is to simply open the file and handle the error if it doesn't exist or isn't accessible.

@imyller
Copy link
Member

imyller commented Sep 18, 2016

Your effort is much appreciated @connium, but I'm -1 on landing this.

What I'd like to see added is the part where err.code is referenced to Common System Errors. That alone would be good improvement to the documentation.

@jasnell
Copy link
Member

jasnell commented Sep 20, 2016

@connium .. the clarification on the error code is definite +1.

@imyller @bnoordhuis ... I think I'd be fine with adding this example so long as there's also a note added saying exactly what @bnoordhuis is describing in his comment... that while this example works, there are better and more reliable ways of accomplishing it. I would definitely sign off on it with that added.

@imyller
Copy link
Member

imyller commented Sep 20, 2016

I agree with @jasnell and I won't object to this PR being landed, if doc is amended with valid points @bnoordhuis made in his comment.

@connium
Copy link
Author

connium commented Oct 4, 2016

@imyller @bnoordhuis @jasnell thanks for your feedback!

I wasn't aware of the complexity of doing file existence checks the right way 😃
I just extended the documentation and removed the example as it looks like there is no silver bullet and I don't want to introduce "bad behavior" 😉

@imyller
Copy link
Member

imyller commented Oct 4, 2016

I think that the doc is much better now.

This adds important err.code reference to Common System Errors and also properly instructs user in implementing file existence checking.

Copy link
Member

@imyller imyller left a comment

Choose a reason for hiding this comment

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

LGTM

Instead, user code should open/read/write the file directly and handle the
error raised if the file is not available.

To check if a file exists without manipulating it afterwards, [`fs.access()`] is recommended.
Copy link
Member

Choose a reason for hiding this comment

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

nit: long line here

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I missed that. It is fixed now.

Copy link
Member

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

jasnell pushed a commit that referenced this pull request Oct 7, 2016
Add an example on how to test if a file exists with fs.stat.
Also add a link to the Common System Errors.

Fixes: https://github.com/nodejs/issues/6752
PR-URL: #8585
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
@jasnell
Copy link
Member

jasnell commented Oct 7, 2016

Landed in 23b9625. Thank you!

@jasnell jasnell closed this Oct 7, 2016
jasnell pushed a commit that referenced this pull request Oct 10, 2016
Add an example on how to test if a file exists with fs.stat.
Also add a link to the Common System Errors.

Fixes: https://github.com/nodejs/issues/6752
PR-URL: #8585
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
Add an example on how to test if a file exists with fs.stat.
Also add a link to the Common System Errors.

Fixes: https://github.com/nodejs/issues/6752
PR-URL: #8585
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

6 participants