Skip to content

Added exists. #5

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

Merged
merged 1 commit into from
Sep 10, 2014
Merged

Added exists. #5

merged 1 commit into from
Sep 10, 2014

Conversation

joneshf
Copy link
Contributor

@joneshf joneshf commented Sep 9, 2014

I'd like to add exists to the Async version as well. However, this is one of the few(only?) function that actually doesn't throw an error as its first argument. So, implementing it means we have a couple of options:

  1. Wrap it into a function that does nothing with its first Error argument.
  2. Drop into the ffi and handle this one case.

The first one seems like a mismatch between the underlying api and this version.
The second one seems a bit unnecessary of a thing to have to do for one specific function, but it brought up some questions in IRC. In specific, how can this function remove the effects safely?: https://github.com/purescript-contrib/purescript-node-fs/blob/master/src/Node/FS/Async.purs#L43-L52 I might be just missing somehting, but it doesn't seem safe.

I don't really care which way is prefered. I really only want the Sync version anyway. So, just let me know which to go with and I'll implement it.

@garyb
Copy link
Member

garyb commented Sep 9, 2014

Aside from this implementation detail, I left exists out because of this, from the documentation you linked to:

fs.exists() is an anachronism and exists only for historical reasons. There should almost never be a reason to use it in your own code.

In particular, checking if a file exists before opening it is an anti-pattern that leaves you vulnerable to race conditions: another process may remove the file between the calls to fs.exists() and fs.open(). Just open the file and handle the error when it's not there.

@joneshf
Copy link
Contributor Author

joneshf commented Sep 10, 2014

I dislike that documentation. It seems short-sighted. It appears to have been written by someone who thinks the reason for a file path is only to read/write to the file. Sometimes you just want to know if the file exists without doing anything immediately with the contents of the file. So reading something and handling the error when it fails just to know if the file exists seems like the real anti-pattern here. I mean, the documentation is basically suggesting using exceptions for control flow.

I understand why you left it out (I would've done the same), as I was scared off by it initially as well. But after thinking about it for a while, and trying to find some other way, it seems to make more sense to just use exists when you actually want to know if a file exists.

@garyb
Copy link
Member

garyb commented Sep 10, 2014

Yeah, I suppose that's fair enough. Using stat or something and catching the error doesn't really seem like a better option than having exists for these situations, as you mentioned.

I'll merge this as it is, and add exists to Async tonight when I fix up the Eff actions evaluating too early. Will probably just use the FFI to do it for this case.

garyb added a commit that referenced this pull request Sep 10, 2014
@garyb garyb merged commit aa0f4a6 into purescript-node:master Sep 10, 2014
@joneshf
Copy link
Contributor Author

joneshf commented Sep 10, 2014

Thanks. If you want me to take care of some of this stuff just let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants