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

os module should have an "exists" function #121

Open
wizzardx opened this issue Jan 15, 2019 · 16 comments
Open

os module should have an "exists" function #121

wizzardx opened this issue Jan 15, 2019 · 16 comments

Comments

@wizzardx
Copy link

The os module currently contains these functions:

  • existsFile
  • existsDir
  • symlinkExists

Which are separate checks for if directory entries (of a given type) exist or not.

the new "exists" function would be a check as to whether any directory entry of that name exists, regardless of type.

@Araq
Copy link
Member

Araq commented Jan 16, 2019

What's the use case?

@wizzardx
Copy link
Author

Typically something like additional sanity checking, in overly-defensively-written code.

eg:

you have a function with a several pre-conditions, one of which is that no file/dir/symlink/whatever that exists with that name.

And a set of pre-conditions at the end of the function including that a file with that name exists, and meets a few other criteria.

It's a pattern that's useful to follow if for instance, you're running a lot of different utility functions under a directory, but want to ensure that - eg - the directory is in an expected state before a given function's logic can run.

I do that kind of thing a fair amount in Python scripts, via the os.path.exists() function. In Nim I currently need to make a wrapper "exists" function that runs all of the existing existsX functions.

@Araq
Copy link
Member

Araq commented Jan 16, 2019

you have a function with a several pre-conditions, one of which is that no file/dir/symlink/whatever that exists with that name.

That's racy and universally regarded as bad style. You need to check for name clashes after an attempt to write it.

@wizzardx
Copy link
Author

It's not racy if other parts of your system are already ensuring a race-free environment. eg, lock file usage that ensures that only a single instance of your service can be working on a service-specific directory structure at a time. This is how linux services work in general.

The pre and post-conditions I'm describing are mainly to deal with detecting bugs or coding issues (eg, assert statements that wouldn't make it into release compiles), rather than the only checks that take place. eg, you'd still get those errors you've described, but maybe only far later on.

Would rather not get stuck on the exact details of my example/use-case, however.

@dom96
Copy link
Contributor

dom96 commented Jan 16, 2019

Can you give some more specific use case examples?

@wizzardx
Copy link
Author

wizzardx commented Jan 16, 2019

A more specific example - converting existing Python code that uses os.path.exists, over to Nim.

One way you'd do that currently in Nim:

import os
let p = "/tmp/help"
if existsFile(p) or existsDir(p):
  echo "File or directory already exists: ", p

One downside is that the 'stat' C function needs to be called twice against the same file.

Also, I think this looks a bit more readable:

import os
let p = "/tmp/help"
if exists(p):
  echo "File or directory already exists: ", p

@Araq
Copy link
Member

Araq commented Jan 17, 2019

proc exists(p: string): bool =
  try:
    discard getFileInfo(p)
    result = true
  except OSError:
    result = false

One stat call. Still not something I want to add to os.nim, it's already so big that you missed getFileInfo.

@wizzardx
Copy link
Author

An error getting file info isn't the same as the file not existing.

For Linux, I think it's a bit more correct to do things this way:

import posix
import os

proc exists(p: string): bool =
  result = true
  try:
    discard getFileInfo(p)
  except OSError as e:
    if e.errorCode == ENOENT:
      result = false
    else:
      raise

I'm not sure what the exact equivalent on none-linux platforms would be.

@dom96
Copy link
Contributor

dom96 commented Jan 17, 2019

The example you gave isn't substantial enough to warrant the addition IMO.

@andreaferretti
Copy link

The fact that a snippet was written by @Araq, it turns out it is not correct, and so it gets in a fixed version by @wizzardx, and even at this point we are not sure whether this is the right version for an OS other than Linux definitely warrants the addition for me!

@Araq
Copy link
Member

Araq commented Jan 17, 2019

Hey, I'm allowed to make mistakes, right?

@andreaferretti
Copy link

Sure, but that's the point! I would have made the same mistake or worse. Even if this function seems trivial, maybe it's not that simple

@Araq
Copy link
Member

Araq commented Jan 18, 2019

The major argument is "it's not often needed", not that it's easy to roll your own.

@al6x
Copy link

al6x commented Mar 28, 2021

Good point for adding exists is for completeness, simplicity and less confusion for people.

Still not something I want to add to os.nim, it's already so big that you missed getFileInfo

Yes, it should not be added to os, as it's already too big. I think the proper solution would be to create fs module. File and IO operations in os and system modules should be deprecated and eventually removed. I'm not saying it should be done now, but maybe consider it for Nim v2.

@FedericoCeratto
Copy link
Member

Reminder: fileExists(foo) or dirExists(foo) does not detect special or device files including named pipes.

@timotheecour
Copy link
Member

this should be named pathExists

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 a pull request may close this issue.

7 participants