- Notifications
You must be signed in to change notification settings - Fork1.4k
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 ourterms of service andprivacy 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
Conversation
…origin)" message when system ruby is in use
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.
Thank you! I can certainly see how "set by FILE" can be misleading if FILE doesn't exist.
However, theset by ~/.rbenv/version message has two meanings:
- The current value wasread from that file. (This is not exactly true if the file is missing; "system" is merely a default value.)
- 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 showsystem (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 withrbenv-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 forrbenv-version-origin.)
What do you think about a compromise along the lines of:
## rbenv-versionversion_name="$(rbenv-version-name)"version_origin="$(rbenv-version-origin)"if ["$version_origin"="${RBENV_ROOT}/version" ]&& [!-e"$version_origin" ];thenecho"$version_name (something useful as fallback here)"elseecho"$version_name (set by$version_origin)"fi
What would be a good fallback message (ideally something other than just no explanation)?
What about something like (to give an example):
? |
@jf Sorry for late reply. Let's just say If we wanted to be more user-friendly, then when the user runs |
I did suggest a message; that didn't work for you? If we just say |
Yours was a good suggestion; I just think that we shouldn't really say Perhaps rbenv should be better at across the board suggesting that a person edits the current version in play by using
Well, following my suggestion above, I think that we can leave version_name="$(rbenv-version-name)"version_origin="$(rbenv-version-origin)"if ["$version_origin"="${RBENV_ROOT}/version" ]&& [!-e"$version_origin" ];thenecho"$version_name"elseecho"$version_name (set by$version_origin)"fi I think this approach is safer because |
ok, I see (sorry, I got a bit confused there). I'm ok with this: should I push another commit with this then? |
@jf Yes please! You may even force-push if you want a cleaner history |
…ection" logic as per@mislav
:) I prefer the history as is; I'm pushing more commits as we speak. |
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.
Looks great, thank you for your patience!
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, andrbenv version-namecan be misleading, and point to a missing rbenv version file.