Skip to content
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

feat(versioning/semver): add semver-coerced versioning #11995

Merged
merged 9 commits into from
Oct 20, 2021
Merged

feat(versioning/semver): add semver-coerced versioning #11995

merged 9 commits into from
Oct 20, 2021

Conversation

olegkrivtsov
Copy link
Contributor

Changes:

This is my work in progress for issue #5623. I decided to push what I have for now to request some help/review.

The task was to wrap inputs with semver.coerce() and consume them to the API exported by the module. I tried to do that, but ran into problems with unit tests. The problems seem to be related to the way semver.coerce() handles the version string. If it finds surrounding text (non-digits), it ignores that text. So, my isValid() function doesn't validate the version string correctly. It accepts ranges as well as malformed versions.

For example:

isValid('v2') => true // correct
isValid('v2foo') => true // incorrect - this is a malformed version
isValid('~v1.0.0`) => true // incorrect - this is a range

Not sure - either I don't understand the task correctly, or semver.coerce() is not good for this task. Could you please review my code and advice the best way to move forward?

Context:

Work in progress #5623

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please tick one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@rarkins
Copy link
Collaborator

rarkins commented Oct 5, 2021

you should not be making any changes to lib/versioning/semver - you should add lib/versioning/semver-coerced

@olegkrivtsov
Copy link
Contributor Author

Thanks @rarkins. That was not clear from the previous discussion, sorry. I'll do as requested.

@olegkrivtsov
Copy link
Contributor Author

Could you please review this PR? I've tried to implement the semver-coerced versioning as requested.

@rarkins
Copy link
Collaborator

rarkins commented Oct 8, 2021

Please use the "request review" option in GitHub in future - no need for comments

lib/versioning/semver-coerced/readme.md Outdated Show resolved Hide resolved
lib/versioning/semver-coerced/index.ts Outdated Show resolved Hide resolved
lib/versioning/semver-coerced/index.ts Outdated Show resolved Hide resolved
lib/versioning/semver-coerced/index.ts Show resolved Hide resolved
@olegkrivtsov olegkrivtsov requested a review from rarkins October 8, 2021 10:23
lib/versioning/semver-coerced/index.ts Outdated Show resolved Hide resolved
lib/versioning/semver-coerced/index.ts Outdated Show resolved Hide resolved
lib/versioning/semver-coerced/readme.md Outdated Show resolved Hide resolved
olegkrivtsov and others added 2 commits October 11, 2021 18:16
Co-authored-by: Rhys Arkins <rhys@arkins.net>
@olegkrivtsov olegkrivtsov requested a review from rarkins October 12, 2021 06:00
@rarkins rarkins enabled auto-merge (squash) October 13, 2021 09:48
@rarkins rarkins merged commit 7cc72c7 into renovatebot:main Oct 20, 2021
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 28.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@olegkrivtsov
Copy link
Contributor Author

Hi @rarkins could you please also close #5623 since this PR was closed?

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants