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

Change the error behavior of Sys.which #27284

Closed
ararslan opened this issue May 27, 2018 · 6 comments
Closed

Change the error behavior of Sys.which #27284

ararslan opened this issue May 27, 2018 · 6 comments
Labels
breaking This change will break code filesystem Underlying file system and functions that use it

Comments

@ararslan
Copy link
Member

ararslan commented May 27, 2018

I have two suggestions for Sys.which, which was added in #26559:

  1. Make it both accept and return Cmds rather than Strings.

  2. Return nothing when the given command is not found rather than throwing an error. (This was actually suggested by someone else in that PR.)

As a motivation for this change, consider the case of selecting an executable from a list of candidates. With the above suggestions, one could write this

tar = coalesce(Sys.which(`tar`), Sys.which(`gtar`))

and use the value of the resulting variable directly in commands, pipeline, etc. Currently this must be written as

tar = try
    Sys.which("tar")
catch
    try
        Sys.which("gtar")
    catch
        error(":(")
    end
end

which is significantly less ergonomic.

@ararslan ararslan added breaking This change will break code filesystem Underlying file system and functions that use it triage This should be discussed on a triage call labels May 27, 2018
@StefanKarpinski
Copy link
Member

StefanKarpinski commented May 28, 2018

Definitely agree about returning nothing if a command doesn’t exist. However, I think I disagree with the part about returning a Cmd. It’s a path and the canonical representation of a path is a string; a Cmd is a kind of "vector of words". We should make sure that nothing throws an error when spliced into a Cmd though (it may already).

@elextr
Copy link

elextr commented May 28, 2018

If sys.which("tar") returns a Cmd for the program tar, how are the arguments then specified?

@ararslan
Copy link
Member Author

how are the arguments then specified?

Interpolation

run(`$(Sys.which("tar")) xzf thingo.tar.gz`)

It’s a path and the canonical representation of a path is a string; a Cmd is a kind of vector of words.

True, though in this case the path is a path to an executable, which is likely mostly useful in Cmds, I would think.

@StefanKarpinski
Copy link
Member

Oops, nothing does not throw an error when spliced into a command:

julia> `$nothing`
`nothing`

That should certainly be fixed since it is unlikely that running a command called nothing is what is intended; it's a bit ad hoc but I think it's the right thing to do here.

@StefanKarpinski
Copy link
Member

True, though in this case the path is a path to an executable, which is likely mostly useful in Cmds, I would think.

The fact that it's a path to an executable is irrelevant. The key fact is that it's not a collection of words like a Cmd is, it's always just one path. E.g. if there's a space in the path to the executable, you have to go to extra trouble to make sure it's not split there; with a string you get the right behavior without doing anything.

@ararslan ararslan changed the title Change return type and error behavior of Sys.which Change the error behavior of Sys.which May 28, 2018
ararslan added a commit that referenced this issue May 28, 2018
Currently `Sys.which("prog")` will throw an error if `prog` is not
found or is found but is not executable. This changes the function to
instead return `nothing` in those cases, thereby simplifying logic for
situations such as hierarchically choosing an executable based on which
in a list is present.

Fixes #27284.
ararslan added a commit that referenced this issue May 28, 2018
Currently `Sys.which("prog")` will throw an error if `prog` is not
found or is found but is not executable. This changes the function to
instead return `nothing` in those cases, thereby simplifying logic for
situations such as hierarchically choosing an executable based on which
in a list is present.

Fixes #27284.
@Keno Keno removed the triage This should be discussed on a triage call label May 31, 2018
@Keno
Copy link
Member

Keno commented May 31, 2018

There seems to be broad agreement on what to do, so de-triaging. Whether this can make it into master before the release can be discussed in #27298.

staticfloat pushed a commit that referenced this issue May 31, 2018
Currently `Sys.which("prog")` will throw an error if `prog` is not
found or is found but is not executable. This changes the function to
instead return `nothing` in those cases, thereby simplifying logic for
situations such as hierarchically choosing an executable based on which
in a list is present.

Fixes #27284.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code filesystem Underlying file system and functions that use it
Projects
None yet
Development

No branches or pull requests

4 participants