-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Make Sys.which return nothing instead of throwing #27298
Conversation
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.
This is good, but I echo @StefanKarpinski's thought that we should do something special when you try to interpolate If there's some way for us to specialize, I think it would be nice to do one of two things:
If we do nothing and leave this as is, I guarantee there will be questions about why |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes as they are look good, but let's figure out what we want to do with `$(Sys.which("foo"))`
The latter option (throw an error) seems like the clearly correct choice to me. If you want to splice in an empty string, use an empty string. If you ended up with |
It's possible it should also be an error to pass |
I do think that's the most principled solution. |
So I guess this is 2.0 material now that 0.7-alpha is tagged? |
Meh, we can sneak in a couple of things between alpha and beta. This won't be the only thing and it only affects a rather rarely used API. |
Maybe we can sneak this in during the alpha period? |
Should the |
I say we merge this, and continue |
Issue opened here: #27352 and with that I'm going to merge, as this is an improvement on a change that didn't even exist in 0.6, so it's not even really breaking. ;) |
Currently
Sys.which("prog")
will throw an error ifprog
is not found or is found but is not executable. This changes the function to instead returnnothing
in those cases, thereby simplifying logic for situations such as hierarchically choosing an executable based on which in a list is present.Fixes #27284.