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

libexec/rbenv-version: get rid of misleading "set by $(rbenv-version-origin)" message when system ruby is in use #1203

Merged
merged 5 commits into from
Jan 16, 2020

Conversation

jf
Copy link
Contributor

@jf jf commented Jan 8, 2020

as per message: it can be that when the system ruby is in use that there simply is no rbenv version file. However, the output of rbenv version, rbenv versions, and rbenv version-name can be misleading, and point to a missing rbenv version file.

Copy link
Member

@mislav mislav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I can certainly see how "set by FILE" can be misleading if FILE doesn't exist.

However, the set by ~/.rbenv/version message has two meanings:

  1. The current value was read from that file. (This is not exactly true if the file is missing; "system" is merely a default value.)
  2. Writing to that file allows you to change the version. This is true regardless of whether the file exists or not.

If we don't show system (set by ${RBENV_ROOT}/version) by default anymore, I worry that people will not understand that this file, if created, dictates the global version.

Additionally, I'm not comfortable with rbenv-version-origin possibly yielding no output. The caller is then forced to guess what the no-output case means. (Reminder that plugins and other scripts may also consume the output for rbenv-version-origin.)

What do you think about a compromise along the lines of:

## rbenv-version

version_name="$(rbenv-version-name)"
version_origin="$(rbenv-version-origin)"

if [ "$version_origin" = "${RBENV_ROOT}/version" ] && [ ! -e "$version_origin" ]; then
  echo "$version_name (something useful as fallback here)"
else
  echo "$version_name (set by $version_origin)"
fi

What would be a good fallback message (ideally something other than just no explanation)?

@jf
Copy link
Contributor Author

jf commented Jan 12, 2020

What about something like (to give an example):

system (default; use /home/jf/.rbenv/version to set)

?

@mislav
Copy link
Member

mislav commented Jan 14, 2020

@jf Sorry for late reply. Let's just say system (nothing in parentheses) for now if RBENV_ROOT/version is missing, since I can't think of anything better.

If we wanted to be more user-friendly, then when the user runs rbenv version and no global version is set, we can suggest that they set a global version other than "system" (since they likely don't want to be using "system" when installing gems). But that's a totally different feature and probably out scope for your original fix.

@jf
Copy link
Contributor Author

jf commented Jan 14, 2020

I did suggest a message; that didn't work for you?

If we just say system with no message after, we're back to rbenv-version-origin returning nothing, fyi.

@mislav
Copy link
Member

mislav commented Jan 15, 2020

I did suggest a message; that didn't work for you?

Yours was a good suggestion; I just think that we shouldn't really say use /home/jf/.rbenv/version to set because we don't expect the user to write to this file directly - instead we want them to use rbenv global <version>.

Perhaps rbenv should be better at across the board suggesting that a person edits the current version in play by using rbenv global, rbenv local, and rbenv shell. But that's an improvement for another PR.

If we just say system with no message after, we're back to rbenv-version-origin returning nothing, fyi.

Well, following my suggestion above, I think that we can leave rbenv-version-origin intact as before your change, and instead implement this in libexec/rbenv-version:

version_name="$(rbenv-version-name)"
version_origin="$(rbenv-version-origin)"

if [ "$version_origin" = "${RBENV_ROOT}/version" ] && [ ! -e "$version_origin" ]; then
  echo "$version_name"
else
  echo "$version_name (set by $version_origin)"
fi

I think this approach is safer because rbenv version is meant more for human consumption, so we can tweak it, and rbenv-version-origin could be potentially used in scripts, so it's better to leave the latter intact.

@jf
Copy link
Contributor Author

jf commented Jan 15, 2020

ok, I see (sorry, I got a bit confused there). I'm ok with this: should I push another commit with this then?

@mislav
Copy link
Member

mislav commented Jan 15, 2020

@jf Yes please! You may even force-push if you want a cleaner history

@jf
Copy link
Contributor Author

jf commented Jan 16, 2020

:) I prefer the history as is; I'm pushing more commits as we speak.

Copy link
Member

@mislav mislav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you for your patience!

@mislav mislav merged commit 7795476 into rbenv:master Jan 16, 2020
gioele pushed a commit to gioele/rbenv that referenced this pull request Feb 15, 2023
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