Skip to content

Use semver package for semantic version comparison and precedence checking#427

Merged
henrymercer merged 7 commits intogithub:masterfrom
henrymercer:fix-semver-comparison
Jun 2, 2020
Merged

Use semver package for semantic version comparison and precedence checking#427
henrymercer merged 7 commits intogithub:masterfrom
henrymercer:fix-semver-comparison

Conversation

@henrymercer
Copy link
Copy Markdown
Contributor

@henrymercer henrymercer commented Jun 1, 2020

This PR refactors the CLI distribution manager to use the semver package. This:

  • fixes an issue with precedence checking where versions with prereleases were not compared correctly with versions without prereleases (CLI version sorting potentially wrong #424)
  • improves handling of cases where users have opted-in then opted-out of prereleases (see commit message of 1bc50f6 and the inline code comment for details)
  • simplifies defining what versions of the CLI are compatible with the extension
  • removes code implementing semver parsing and precedence checking

Other points to note:

  • Semantic versions are parsed using semver.parse rather than semver.coerce since semver.coerce strips build metadata.
  • Semantic versions are formatted using version.raw since this includes build metadata.
  • Release creation dates are now compared using the en-US locale to remove potential locale dependence of version comparison.

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • @github/product-docs-dsp has been cc'd in all issues for UI or other user-facing changes made by this pull request.

Fixes #424

Suppose a user has the includePrereleases config option set, installs an
extension-managed prerelease, then decides they no longer want
prereleases and disables includePrereleases.
In this case, we should prompt the user to downgrade the CLI to a
non-prerelease version.
However, if the user is managing their own CLI, we will allow them to
use prereleases without incompatibility prompts.
aeisenberg
aeisenberg previously approved these changes Jun 2, 2020
Copy link
Copy Markdown
Contributor

@aeisenberg aeisenberg 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. Just a couple of stylistic and wording suggestions that you can take or leave.

Comment thread extensions/ql-vscode/src/distribution.ts Outdated
Comment thread extensions/ql-vscode/src/distribution.ts Outdated
Comment thread extensions/ql-vscode/src/extension.ts Outdated
Comment thread extensions/ql-vscode/src/extension.ts Outdated
Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>
@henrymercer
Copy link
Copy Markdown
Contributor Author

henrymercer commented Jun 2, 2020

Thanks @aeisenberg, I agree with and have applied your suggestions.

jcreedcmu
jcreedcmu previously approved these changes Jun 2, 2020
Copy link
Copy Markdown
Contributor

@adityasharad adityasharad 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 overall! Some suggestions for clarity (not blocking) and test coverage.

Comment thread extensions/ql-vscode/src/distribution.ts Outdated
Comment thread extensions/ql-vscode/src/distribution.ts
Comment thread extensions/ql-vscode/src/vscode-tests/no-workspace/distribution.test.ts Outdated
Comment thread extensions/ql-vscode/src/vscode-tests/no-workspace/distribution.test.ts Outdated
@henrymercer henrymercer merged commit 71f74cb into github:master Jun 2, 2020
@henrymercer henrymercer deleted the fix-semver-comparison branch June 2, 2020 21:01
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.

CLI version sorting potentially wrong

4 participants