Fix dvc details version incorrectly showing "Not Found" #3787
Conversation
extension/src/setup/index.ts
Outdated
| return { | ||
| command, | ||
| version: cwd ? await this.getCliVersion(cwd) : undefined | ||
| version: version || (await this.getCliVersion(cwd, true)) |
There was a problem hiding this comment.
Looking at the discovery.ts code, we check for the version, then check for a global version. Our function that gets DVC CLI details, however, wasn't checking for a global.
This could possibly be why Ivan was seeing Version: Not Found but the section was saying DVC Incompatible.
There was a problem hiding this comment.
Why not save the version when setup gets run instead of recreating the logic here?
There was a problem hiding this comment.
I had a quick look at the code and potentially setCliCompatible in Setup could be passed the version/undefined, it could then set both the version and cliCompatible. The version can then be passed to the webview without having to call this.getCliVersion each time.
There was a problem hiding this comment.
I assume the current behaviour is why we are calling dvc version all of the time too:
Screen.Recording.2023-05-01.at.12.25.15.pm.mov
There was a problem hiding this comment.
Thanks for the ideas, Matt! Updated the code to use a variable for version.
| @@ -579,7 +578,6 @@ suite('Setup Test Suite', () => { | |||
| mockRunSetup.restore() | |||
| stub(config, 'isPythonExtensionUsed').returns(false) | |||
| stub(config, 'getPythonBinPath').resolves(join('python')) | |||
| stub(setup, 'getDvcCliDetails').resolves(undefined) | |||
There was a problem hiding this comment.
With getDvcCliDetails no longer calling getDvcVersion, we can delete related stubs.
|
Code Climate has analyzed commit c46303f and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (85% is the threshold). This pull request will bring the total coverage in the repository to 94.6% (0.0% change). View more on Code Climate. |
getDvcCliDetails()rely on a version variable instead of callinggetCliVersionPossibly will fix the bug mentioned in #3434 (comment)