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

Support Revision Part of version number #962

Closed
wants to merge 29 commits into from
Closed

Conversation

Juff-Ma
Copy link

@Juff-Ma Juff-Ma commented Jan 7, 2024

Pull request for #961

One test couldn't complete for me because of a weird error with the nupkg being put in the wrong folder. One still can't run (seemingly because of the same issue). Another one is impossible for me to run (DoesNotRecreatePackage) because my system language is set to german.

All other test run fine for me. (Please ignore the double and coauthored commit, this is an error caused by visual studio git support)

Your Name and others added 28 commits January 7, 2024 16:29
…seems to auto-trucate to 3 part if no revision is specified
…seems to auto-trucate to 3 part if no revision is specified
@Juff-Ma Juff-Ma changed the title C Support Revision Part of version number Jan 7, 2024
@adamralph
Copy link
Owner

@Juff-Ma one important thing to bear in mind is that the experience for consumers who are using SemVer (3 part) versions should be unaffected by this change. This spike seems to assume that only NuGet (4 part) versions are used and it appears to force that on to all consumers.

@Juff-Ma
Copy link
Author

Juff-Ma commented Jan 8, 2024

@Juff-Ma one important thing to bear in mind is that the experience for consumers who are using SemVer (3 part) versions should be unaffected by this change. This spike seems to assume that only NuGet (4 part) versions are used and it appears to force that on to all consumers.

The only thing it changes currently is the default version for auto increment but i think this could be changed to Patch by default again if required. I added tests (left some old ones intact) to make sure that 3-part tags would still work.

If someone was to use the tag "v1.2.3" and auto-increment Patch it should only change the logging output now printing 1.2.3.0/1.2.4.0 this seems to be possible because minver/nugetversion by default doesn't assume what version format you use but i could be wrong

@adamralph
Copy link
Owner

@Juff-Ma thanks for providing the spike.

I get the feeling it would have to be controlled by an option, e.g. <MinVerFourPartVersion>true</MinVerFourPartVersion> to ensure that the default behaviour does not mention a four part version anywhere—not even in the logs—otherwise it could be quite confusing. This likely means maintaining two code paths—one for three part versions and one for four part versions, which seems non-trivial.

Moreover, I'm not sure the use case is common enough to make this worthwhile. It seems that three part versions are canonical. The System packages all seem to use three part versions and in the first few pages of the most popular packages, only the AWS SDK packages use a four part version.

Given the above, I'm not sure it makes sense add this feature.

@Juff-Ma
Copy link
Author

Juff-Ma commented Jan 8, 2024

@Juff-Ma thanks for providing the spike.

I get the feeling it would have to be controlled by an option, e.g. <MinVerFourPartVersion>true</MinVerFourPartVersion> to ensure that the default behaviour does not mention a four part version anywhere—not even in the logs—otherwise it could be quite confusing. This likely means maintaining two code paths—one for three part versions and one for four part versions, which seems non-trivial.

Moreover, I'm not sure the use case is common enough to make this worthwhile. It seems that three part versions are canonical. The System packages all seem to use three part versions and in the first few pages of the most popular packages, only the AWS SDK packages use a four part version.

Given the above, I'm not sure it makes sense add this feature.

That makes sense. The use cases i have are pretty niche and even i don't have them all the time. While it would have been cool to have this included it isn't a requirement even for me. I get that having to maintain two code paths for such a niche functionality is unacceptable. I personally also use SemVer when applicable and therefore also MinVer when available .

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.

2 participants