Skip to content

feat: print requested version on fail , closes #2033#2034

Open
hfi-wiit wants to merge 1 commit intoasdf-vm:masterfrom
hfi-wiit:requested_version
Open

feat: print requested version on fail , closes #2033#2034
hfi-wiit wants to merge 1 commit intoasdf-vm:masterfrom
hfi-wiit:requested_version

Conversation

@hfi-wiit
Copy link
Copy Markdown

As requested in #2033, this PR just adds a few lines to print the requested version in the event the related binary version is not installed.

@hfi-wiit hfi-wiit requested a review from a team as a code owner March 21, 2025 18:35
@hfi-wiit hfi-wiit changed the title print requested version on fail , closes #2033 feat: print requested version on fail , closes #2033 Mar 21, 2025
@carlqt
Copy link
Copy Markdown

carlqt commented Mar 7, 2026

Unsure what we need to do to get this merged. It's a very simple fix and the change is appreciated. I hate the current message

Comment thread internal/shims/shims.go

if len(existingPluginToolVersions) == 0 {
return "", plugins.Plugin{}, "", false, NoVersionSetError{shim: shimName}
return "", plugins.Plugin{}, requestedVersion, false, NoVersionSetError{shim: shimName}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this should only return a version as the 3 value if a matching one is found. Is there a reason this return value was changed?

Copy link
Copy Markdown
Member

@Stratus3D Stratus3D left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @hfi-wiit ! Sorry for the late review, I'm only now catching up on PRs. The changes appear good, but we need tests before we merge to ensure this works as expected in all cases and so to prevent future regressions. I'll circle back to this in about a week and write tests then. In the meantime you are welcome to contribute tests if you want.

@Stratus3D
Copy link
Copy Markdown
Member

Also I think we may want to address #1467 in this as well.

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.

3 participants