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

Handle case where nodenv-commands --sh returns nothing #217

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aphedges
Copy link

In exceptional cases (e.g., custom installation, malfunctions elsewhere), nodenv-commands --sh may return nothing. In non-Fish shells, this would cause "syntax error near unexpected token )'" in nodenv()`.

Bash does not allow one to specify a case option without a pattern. We work around it by defaulting to /. Commands, being filenames, can never match it. In Fish, nothing needs to be done: it apparently does interpret a case without an argument as one that never matches.

This patch is based on 02e1d4a1 from pyenv, which was written by @ native-api and I.


Feel free to edit the commit message as needed, but I would appreciate if you kept most of it. I sometimes find it useful to grep the Git logs when developing.

If you want to see some discussion about the original patch for pyenv, look at pyenv/pyenv#2908.

Footnotes

  1. https://github.com/pyenv/pyenv/commit/02e1d4a293139ecf2c94206ee9c00b388550366e

In exceptional cases (e.g., custom installation, malfunctions
elsewhere), `nodenv-commands --sh` may return nothing. In non-Fish
shells, this would cause "syntax error near unexpected token `)'" in
`nodenv()`.

Bash does not allow one to specify a `case` option without a pattern. We
work around it by defaulting to `/`. Commands, being filenames, can
never match it. In Fish, nothing needs to be done: it apparently does
interpret a `case` without an argument as one that never matches.

This patch is based on 02e1d4a[^1] from `pyenv`, which was written by
@ native-api and I.

[^1]: pyenv/pyenv@02e1d4a
@jasonkarns
Copy link
Member

Is there a corresponding PR for rbenv?

@aphedges
Copy link
Author

@jasonkarns, no, not unless someone else made one. I can do that, if you require patches to be upstream first.

@jasonkarns
Copy link
Member

We don't require it, no. But I wanted to make sure the work isn't duplicated since we do pull from upstream somewhat regularly.

If you're comfortable opening it there, it's easier to start in the rbenv repo. If not, we can leave it here and I'll see about moving it upstream later.

@aphedges
Copy link
Author

I can make a PR in rbenv soon to better match your workflow. I'm more used to working with pyenv, which I don't think has had changes merged from rbenv in years.

If it gets accepted upstream, then I'll close this PR.

@jasonkarns
Copy link
Member

Thank you, that would be helpful. Even if it gets merged upstream, it's probably best to keep this issue open here until that fix gets pulled downstream as well.

(And we may merge this one in advance anyway once I get a chance to dig into it)

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.

None yet

3 participants