Skip to content

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

doc/api/fs.md Outdated
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