-
Notifications
You must be signed in to change notification settings - Fork 816
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
fix: use version in home dir when no version found in root dir #1883
Conversation
* Use version in home dir when no version found in root dir * Correct `FindExecutable` function so returns `NoVersionSetError` even when `.tool-version` file located * Update `resolve` and `versions` package tests to use test plugin name Fixes #1860
d686f8c
to
864b120
Compare
break | ||
} | ||
|
||
versions, found, err = findVersionsInDir(conf, plugin, homeDir) |
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.
This is the actual fix for the bug.
func TestVersion(t *testing.T) { | ||
testDataDir := t.TempDir() | ||
currentDir := t.TempDir() | ||
conf := config.Config{DataDir: testDataDir, DefaultToolVersionsFilename: ".tool-versions", ConfigFile: "testdata/asdfrc"} | ||
_, err := repotest.InstallPlugin("dummy_plugin", conf.DataDir, "lua") |
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.
Tests were failing for me locally because I actually have the lua plugin installed on my computer. With the change above to make the home directory version the default these tests started picking up my own personal config, so I changed to a test plugin name.
existingPluginToolVersions[plugin] = versions | ||
if len(versions.Versions) > 0 { | ||
existingPluginToolVersions[plugin] = versions | ||
} |
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 code was setting a tool and version even when tempVersions
was empty, which was wrong.
// If no version found, try current users home directory. I'd like to | ||
// eventually remove this feature. |
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.
I'd like to eventually remove this feature.
@Stratus3D It seems very useful — can you share why you want to remove it? If you've already elucidated elsewhere, feel free to just link me.
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.
@jsejcksn the more code I can remove or improve, the more maintainable this project becomes. In the code this is very clearly a "special case" and not necessary for typical. Can you share why you find it useful? I'm still formulating a plan to remove this and replace it with nothing. I've not written out anything yet.
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.
Can you share why you find it useful?
@Stratus3D I find it useful to be able to specify "default" versions for installed tools without needing to place a configuration file outside my home directory.
FindExecutable
function so returnsNoVersionSetError
even when.tool-version
file locatedresolve
andversions
package tests to use test plugin nameFixes #1860