Skip to content

Conversation

@shcheklein
Copy link
Contributor

@shcheklein shcheklein commented Jul 20, 2022

Fixes #2063

  • Fixes tests (we were not testing properly since we were using join(',') instead of join('.') which gives NaNs ... and NaNs are terrible in JS
  • Introduces the variable that defines the latest version of DVC that we know is compatible with. Now if the detected version is between MIN_VERSION and BUILD_VERSION we are not firing any warnings

Things to improve / discuss:

  • Name of the variable. Can be TESTED_WITH_VERSION or something. Any thoughts?
  • Lifecycle of this variable. Probably we should move it to the package.json, listen to DVC updates and automatically release a new version of the extension (can be done in the future, I guess). For now we can keep an eye and see if that works.
  • I'm not familiar with TS - can semver be wrapped to avoid things like NaNs, etc? (minor, I think)

TODO:

  • Address feedback
  • CodeClimate - see the complaints and fix

Copy link
Contributor

@mattseddon mattseddon left a comment

Choose a reason for hiding this comment

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

👍🏻

@shcheklein shcheklein force-pushed the fix-2063 branch 3 times, most recently from 7f49ba7 to 01ee1c4 Compare July 20, 2022 05:31
@qlty-cloud-legacy
Copy link

Code Climate has analyzed commit dd95a6b 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 96.7% (0.0% change).

View more on Code Climate.

Copy link
Contributor

@julieg18 julieg18 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@shcheklein shcheklein merged commit 9d1deab into main Jul 20, 2022
@shcheklein shcheklein deleted the fix-2063 branch July 20, 2022 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CLI version not matching need of the extension

3 participants