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

Restrict versions to SemVer 2.0 only #720

Merged
merged 1 commit into from
Apr 3, 2022
Merged

Restrict versions to SemVer 2.0 only #720

merged 1 commit into from
Apr 3, 2022

Conversation

adamralph
Copy link
Owner

@adamralph adamralph commented Mar 28, 2022

Use case(s)

Currently it is possible to use versions which are not valid SemVer 2.0. For example:

  • 01.002.0003—the leading zeros are not valid SemVer 2.0. MinVer strips them out, and returns the version 1.2.3, which is valid SemVer. This version may be consumed when using either the MinVer or minver-cli package.
  • 1.2.3-beta.0004—the leading zeros are not valid SemVer 2.0. MinVer accepts them, but when using the MinVer package, the SDK rejects the version and an error occurs. It is only possible to consume the version when using the minver-cli package.
  • 01.002.0003-beta.00004—MinVer strips the leading zeros from the major, minor, and patch parts, but not from the pre-release identifiers, and returns the version 1.2.3-beta00004. Because that version is not valid SemVer, when using the MinVer package, the SDK rejects the version and an error occurs. It is only possible to consume the version when using the minver-cli package.
  • 1.2.3-beta.4?—anything but ASCII alphanumerics and hyphens [0-9A-Za-z-] in pre-release identifiers are not valid SemVer. MinVer accepts them, but when using the MinVer package, the SDK rejects the version and an error occurs. It is only possible to consume the version when using the minver-cli package.
  • 1.2.3-beta.4+build.5?—anything but ASCII alphanumerics and hyphens [0-9A-Za-z-] in build metadata identifiers are not valid SemVer. MinVer accepts them, but when using the MinVer package, the SDK rejects the version and an error occurs. It is only possible to consume the version when using the minver-cli package.

Description

With this enhancement, MinVer only accepts valid SemVer 2.0 versions. This means any invalid SemVer 2.0 versions are ignored, avoiding the errors described above, and leading zeros are treated consistently, regardless of whether they are in the major, minor, or patch parts, or in the pre-release identifiers or build metadata—any version containing leading zeros in numeric identifiers are ignored.

This is a breaking change for both the MinVer and minver-cli packages, because versions with leading zeros only in the major, minor, or patch parts are now ignored. Previously, the leading zeros in those parts were stripped, and the versions were used.

This is also a breaking change for the minver-cli package in that versions with leading zeros in numeric pre-release identifiers or numeric build metadata identifiers are now ignored, as are versions with anything but ASCII alphanumerics and hyphens [0-9A-Za-z-] in those identifiers.

Alternatives

Continue with the current behaviour.

Additional context

The changes also revert #718.

Discussed in #719

Originally posted by adamralph March 25, 2022
Currently, leading zeros are removed from the major, minor and patch parts, but they are preserved in pre-release identifiers.

For example, the tag 01.02.03-beta.0004 is parsed as version 1.2.3-beta.0004. This doesn't feel right. It seems logical to either parse as version 01.02.03-beta.0004 (preserving all leading zeros) or as version 1.2.3-beta.4 (preserving no leading zeros).

But that leads into a wider question: both convention and SemVer 2.0 put restrictions on which characters can and can't be in a specific version part. I cannot recall seeing anyone using leading zeros in major, minor, or patch parts, and SemVer disallows it, which generally makes it "OK" that MinVer strips out those leading zeros. So effectively, MinVer is restricting the version to some parts of SemVer (and general convention), and transforming some parts of the version to comply on the consumer's behalf. This also feels off. It should either be completely restrictive, or completely unrestrictive (possibly with an option to switch modes). And if it is restrictive, what better standard to use than SemVer? Also, what does "completely unrestrictive" mean? There would probably still have to be some restrictions, and not just free text.

I'm capturing this in a discussion for now because I'm not sure whether this discussion is going to reveal bugs or features, nor how many of each. There may also be two (or more) separate concerns here but I can't disentangle them yet.

Thanks to @bording for bringing this to my attention.

@adamralph adamralph added the enhancement New feature or request label Mar 28, 2022
@adamralph adamralph added this to the 3.2.0 milestone Mar 28, 2022
@adamralph adamralph force-pushed the semver branch 3 times, most recently from 0b84714 to b53df3c Compare March 28, 2022 07:18
@adamralph adamralph added the breaking This change could break current consumers label Mar 28, 2022
@adamralph adamralph changed the title restrict versions to SemVer 2.0 only Restrict versions to SemVer 2.0 only Mar 29, 2022
@adamralph adamralph marked this pull request as ready for review March 29, 2022 16:03
@bording
Copy link
Collaborator

bording commented Mar 30, 2022

You seem to have left out an alternative option, and one that I personally find more appealing:

Require SemVer 2.0 versions, but first remove any leading zeros found before determining if it is valid.

This still leads to a consistent treatment, and is less disruptive overall. It would also help with migrating from other versioning tools, for example GitVersion.

@adamralph
Copy link
Owner Author

adamralph commented Mar 30, 2022

@bording thanks for the input.

I guess migrating from other tools which do remove leading zeros may be a valid use case for removing the zeros in MinVer.

I'm hesitant to make that the default behaviour because I see no use case for writing those leading zeros now, and it also allows the same version to be specified in many ways (e.g. 1.2.3-beta.0001 or 1.2.3-beta.1).

How about if it was an option?

<MinVerIgnoreLeadingZeros>true</MinVerIgnoreLeadingZeros>

Unfortunately it makes the implementation quite a bit more complicated, since right now (in this PR) I'm just handing the tag names to NuGet.Versioning.SemanticVersion.TryParse. To implement zeros removal, I'd have to get back into parsing the version in MinVer. Although I have to admit, I'm still not very comfortable with the idea of owning a dependency on NuGet.Versioning.

@adamralph
Copy link
Owner Author

adamralph commented Mar 30, 2022

I guess the option could also be implemented by performing a "pre-parse", before NuGet.Versioning.SemanticVersion.TryParse:

  • split the tag name on the first +
  • split the first string on the first -
  • split all three strings on .
  • for any strings that are numeric, remove the leading zeros
  • stitch all the strings back together again, using ., -, and + appropriately

@adamralph
Copy link
Owner Author

adamralph commented Apr 1, 2022

I pushed another commit to add the MinVerIgnoreLeadingZeros option using the method described in #720 (comment)

@bording
Copy link
Collaborator

bording commented Apr 1, 2022

Having it as an option at least does seem to be a good idea. Without it, you could be stuck needing to recreate tags in a new format.

reverts 9f7dd6f

replace Concat with Append for single item
@adamralph
Copy link
Owner Author

I'm moving the addition of MinVerIgnoreLeadingZeros to a separate PR.

@adamralph
Copy link
Owner Author

released in 4.0.0-alpha.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change could break current consumers enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants