Proposal: Adding System.SemanticVersion

NickCraver commented 7 years ago

AB#1115515 One of the issues I keep running into with the "default open" world: prereleases. As more and more projects move to semantic versioning, it'd be nice to have a built-in type to handle these. Today, this fails:

Version.Parse("1.0.0-beta1");

Because System.Version has no concept of pre-release suffixes. I'm not arguing it should be added (that's maybe a little to breaking).

Instead how about a new type? System.SemanticVersion The main property most people would be interested in is the prerelease label, but other tenants of semantic versioning should be included. Comparisons being a big one. A lot of devs get this wrong on corner cases. I was looking for how to do best structure this and noticed: NuGet is already doing it. Here's their SemanticVersion class: (part 1, part2).

The API likely needs a little refinement for general use in the BCL, but is there a desire for this from others? I have use cases from parsing package versions to determining which version of Elasticsearch a client is connected to. There's a broad spectrum of semantic versioning use cases now.

Suggestions from others (collating here):

It should be comparable to Version (additional operator overloads)

API

namespace System 
{
    public class SemanticVersion : IFormattable, IComparable, IComparable<SemanticVersion>, IEquatable<SemanticVersion>, ICloneable
    {
        public SemanticVersion(int major, int minor, int patch);
        public SemanticVersion(int major, int minor, int patch, string prereleaseLabel);
        public SemanticVersion(int major, int minor, int patch, string prereleaseLabel, string metadata);
        public SemanticVersion(int major, int minor, int patch, IEnumerable<string> prereleaseLabel, IEnumerable<string> metadata);
        public SemanticVersion(string version);

        protected SemanticVersion(Version version, string prereleaseLabel = null, string metadata = null);
        protected SemanticVersion(Version version, IEnumerable<string> prereleaseLabels, IEnumerable<string> metadata);

        public int Major { get; }
        public int Minor { get; }
        public int Patch { get; }
        public virtual IEnumerable<string> PrereleaseLabels { get; }
        public virtual bool IsPrerelease { get; }
        public virtual IEnumerable<string> Metadata { get; }
        public virtual bool HasMetadata { get; }

        public virtual string ToString(string format, IFormatProvider formatProvider);

        public override string ToString();
        public override int GetHashCode();

        public virtual SemanticVersion Clone();
        object ICloneable.Clone() => Clone();

        public virtual bool Equals(SemanticVersion other);
        public virtual bool Equals(Version other);

        public virtual int CompareTo(object obj);
        public virtual int CompareTo(SemanticVersion other);
        public virtual int CompareTo(Version other);

        public static SemanticVersion Parse(string versionString);
        public static bool TryParse(string versionString, out SemanticVersion version);

        public static bool operator ==(SemanticVersion left, SemanticVersion right);
        public static bool operator !=(SemanticVersion left, SemanticVersion right);
        public static bool operator <(SemanticVersion left, SemanticVersion right);
        public static bool operator <=(SemanticVersion left, SemanticVersion right);
        public static bool operator >(SemanticVersion left, SemanticVersion right);
        public static bool operator >=(SemanticVersion left, SemanticVersion right);

        public static bool operator ==(Version left, SemanticVersion right);
        public static bool operator !=(Version left, SemanticVersion right);
        public static bool operator <(Version left, SemanticVersion right);
        public static bool operator <=(Version left, SemanticVersion right);
        public static bool operator >(Version left, SemanticVersion right);
        public static bool operator >=(Version left, SemanticVersion right);

        public static explicit operator SemanticVersion(Version version); 

        public static bool operator ==(SemanticVersion left, Version right);
        public static bool operator !=(SemanticVersion left, Version right);
        public static bool operator <(SemanticVersion left, Version right);
        public static bool operator <=(SemanticVersion left, Version right);
        public static bool operator >(SemanticVersion left, Version right);
        public static bool operator >=(SemanticVersion left, Version right);
    }
}

somewhatabstract commented 7 years ago

As an addition, I'd want to see an associated comparer that could properly sort SemanticVersion and Version values together using semantic versioning rules.

karelz commented 7 years ago

Sounds like a good idea. Also quite a few votes (15) in just 5 hours ...

Side question out of curiosity: Are all those 15 folks watching the whole repo or was the issue promoted somewhere (Twitter, MVP summit)?

karelz commented 7 years ago

We need formal API proposal ...

NickCraver commented 7 years ago

I'm talking with the NuGet team tomorrow, I'll try and follow-up on this after. They obviously have some good insights here that are excellent use cases and considerations. I'll try to pitch a formal API (or provide more info) as soon as time allows afterwards.

cdrnet commented 7 years ago

Since a major aspects of the semantics of semantic versioning is about compatibility, it could be worth to also provide basic functionality to check whether an actual semantic version is expected to be compatible to a known version (simply put: same major version, same or larger minor, patch)

karelz commented 7 years ago

Sounds good, let us know how that goes - assigning to you for now as you're working on it. Let me know if that changes.

Structed commented 7 years ago

The issue was indeed shared via Twitter by @NickCraver

karelz commented 7 years ago

@Structed that explains it, thanks!

NickCraver commented 7 years ago

I talked with @yishaigalatzer and @rrelyea at the summit a little about this. I'm not sure they're confident NuGet can use a SemanticVersion in the BCL and I'm not sure either - but let's find out!

Starting with NuGet's current implementation (linked in the issue), here's a first pass at what System.SemanticVersion could look like. The changes are including Version comparison operators and conversions, renaming the comparison enum (this will be something we'll have to find common ground on for NuGet to use those overloads, or add methods). I left updated comments on members I thought may be a little confusing.

My hope is that NuGet's SemanticVersion could simply inherit from this at some point. I'm not sure if that's an achievable goal.

namespace Sysem 
{
    public class SemanticVersion : IFormattable, IComparable, IComparable<SemanticVersion>, IEquatable<SemanticVersion>, IComparable<Version>, IEquatable<Version>
    {
        public SemanticVersion(SemanticVersion version); // Copy
        public SemanticVersion(int major, int minor, int patch);
        public SemanticVersion(int major, int minor, int patch, string releaseLabel);
        public SemanticVersion(int major, int minor, int patch, string releaseLabel, string metadata);
        public SemanticVersion(int major, int minor, int patch, IEnumerable<string> releaseLabels, string metadata);

        protected SemanticVersion(int major, int minor, int patch, int revision, string releaseLabel, string metadata);
        protected SemanticVersion(int major, int minor, int patch, int revision, IEnumerable<string> releaseLabels, string metadata);

        protected SemanticVersion(Version version, string releaseLabel = null, string metadata = null);
        protected SemanticVersion(Version version, IEnumerable<string> releaseLabels, string metadata);

        public int Major { get; }
        public int Minor { get; }
        public int Patch { get; }
        public IEnumerable<string> ReleaseLabels { get; }
        public virtual bool IsPrerelease { get; }
        public virtual string Metadata { get; }
        public virtual bool HasMetadata { get; }

        /// <summary>
        /// Gives a normalized representation of the version, excluding metadata.
        /// </summary>
        public virtual string ToNormalizedString();

        /// <summary>
        /// Gives a full representation of the version including metadata.
        /// </summary>
        public virtual string ToFullString();
        public virtual string ToString(string format, IFormatProvider formatProvider);

        public virtual bool Equals(SemanticVersion other);
        public virtual bool Equals(SemanticVersion other, SemanticVersionComparison versionComparison);
        public virtual bool Equals(Version other);

        public virtual int CompareTo(object obj);
        public virtual int CompareTo(SemanticVersion other);
        public virtual int CompareTo(SemanticVersion other, SemanticVersionComparison versionComparison);
        public virtual int CompareTo(Version other);

        public static bool operator ==(SemanticVersion version1, SemanticVersion version2);
        public static bool operator !=(SemanticVersion version1, SemanticVersion version2);
        public static bool operator <(SemanticVersion version1, SemanticVersion version2);
        public static bool operator <=(SemanticVersion version1, SemanticVersion version2);
        public static bool operator >(SemanticVersion version1, SemanticVersion version2);
        public static bool operator >=(SemanticVersion version1, SemanticVersion version2);

        public static explicit operator SemanticVersion(Version version); 

        public static bool operator ==(SemanticVersion version1, Version version2);
        public static bool operator !=(SemanticVersion version1, Version version2);
        public static bool operator <(SemanticVersion version1, Version version2);
        public static bool operator <=(SemanticVersion version1, Version version2);
        public static bool operator >(SemanticVersion version1, Version version2);
        public static bool operator >=(SemanticVersion version1, Version version2);
    }

    public enum SemanticVersionComparison
    {
        /// <summary>
        /// Semantic version 2.0.1-rc comparison.
        /// </summary>
        Default = 0,
        /// <summary>
        /// Compares only the version numbers.
        /// </summary>
        Version = 1,
        /// <summary>
        /// Include Version number and Release labels in the compare.
        /// </summary>
        VersionRelease = 2,
        /// <summary>
        /// Include all metadata during the compare.
        /// </summary>
        VersionReleaseMetadata = 3
    }
}

Some questions:

Obvious: what's missing?
Overloads: NuGet goes above and beyond this on comparisons, do we need more extension points?
Should an implicit operator to Version be included? I'd think this does more harm than good.
Should conversions to Version fail if prerelease labels are present? If so, how?
Version ranges are a thing, should that be handled in another class? I'd learn towards yes.
What variety of formats should be allowed? I think NuGet's implementation has this pretty well covered already, that may be a lift, functionality wise. There are some allocations we can eliminate though.

Thoughts?

jcolebrand commented 7 years ago

So anything that doesn't match the original Version is now a release-label?

Can't we establish some commonalities around that? alpha, beta, pre, etc. By providing a text enum we start to solidify common business practices and get people on the same stage. You can still have populated release labels but how often are those strings plural?

If you have Version 1.0.9-1pre1 what does that parse into? Are we assuming that 1pre1 is by itself a single release label?

I think that if we're gonna handle sem-ver, let's go ahead and do ranges. Those could be nullable LowerMajor, LowerMinor, LowerPatch as additional fields, and then we could incorporate those into the operator methods. This is because we would assume that your two options are "upper - lower" when you parse things, and that you won't have an array of versions. That would need to be handled with an array of Version or SemVer.

justinvp commented 7 years ago

What are the scenarios for subclassing SemanticVersion? Why not make it sealed like Version and be fully immutable like Version?
Consider a strongly-typed Clone() method that returns SemanticVersion instead of having a copy constructor. SemanticVersion could also implement ICloneable explicitly (privately) with it's ICloneable.Clone() implementation returning the result of calling the strongly-typed Clone(). This would be more consistent with Version which has a Clone() (albeit, not strongly typed) instead of a copy constructor.
Override GetHashCode() (since it overrides Equals() and implements IEquatable<SemanticVersion>)?
Override ToString()?
The operator parameter names should be left and right instead of version1 and version2 according to the framework design guidelines.
What about Parse/TryParse?

jnm2 commented 7 years ago

I agree that this is a BCL must-have. I am a huge proponent of minimalism as long as people's needs aren't blocked.

jnm2 commented 7 years ago

@karelz

Side question out of curiosity: Are all those 15 folks watching the whole repo or was the issue promoted somewhere (Twitter, MVP summit)?

I came from Twitter but now that issues that I care about like this have been brought to my attention, I will be watching the repo from now on.

NickCraver commented 7 years ago

@jcolebrand SemanticVersion is not constrained to those things, so this has to be more flexible per the rules at http://semver.org/ to be generally useful.

I think that if we're gonna handle sem-ver, let's go ahead and do ranges.

I don't disagree...but what's that look like, in API form? Pitch out some ideas?

@justinvp Responses:

What are the scenarios for subclassing SemanticVersion? Why not make it sealed like Version and be fully immutable like Version?

NuGet does additional comparisons in NuGetVersion, you can find it here. Whether it's best for them to subclass this or not is of course up for debate. Maybe sealing it is better, I'm not sure.

Consider a strongly-typed Clone() method that returns SemanticVersion instead of having a copy constructor.

Good idea, that's a better approach. Updated.

Override GetHashCode() (since it overrides Equals() and implements IEquatable<SemanticVersion>)? Override ToString()?

Yes, absolutely. I just left these off for conciseness and left them as assumptions - I'll add them back to be explicit this is happening.

The operator parameter names should be left and right instead of version1 and version2 according to the framework design guidelines.

IMO, neither of those are really clear, but if that's consistent elsewhere in the framework of course we should stay consistent. I'll update.

What about Parse/TryParse?

Well now I just look like an idiot for leaving that off. Adding.

Here's a new revision (should we track this in one place at the top, or does that make comments confusing as it's edited? I can see it both ways):

namespace Sysem 
{
    public class SemanticVersion : IFormattable, IComparable, IComparable<SemanticVersion>, IEquatable<SemanticVersion>, ICloneable
    {
        public SemanticVersion(int major, int minor, int patch);
        public SemanticVersion(int major, int minor, int patch, string releaseLabel);
        public SemanticVersion(int major, int minor, int patch, string releaseLabel, string metadata);
        public SemanticVersion(int major, int minor, int patch, IEnumerable<string> releaseLabels, string metadata);

        protected SemanticVersion(int major, int minor, int patch, int revision, string releaseLabel, string metadata);
        protected SemanticVersion(int major, int minor, int patch, int revision, IEnumerable<string> releaseLabels, string metadata);

        protected SemanticVersion(Version version, string releaseLabel = null, string metadata = null);
        protected SemanticVersion(Version version, IEnumerable<string> releaseLabels, string metadata);

        public int Major { get; }
        public int Minor { get; }
        public int Patch { get; }
        public IEnumerable<string> ReleaseLabels { get; }
        public virtual bool IsPrerelease { get; }
        public virtual string Metadata { get; }
        public virtual bool HasMetadata { get; }

        /// <summary>
        /// Gives a normalized representation of the version, excluding metadata.
        /// </summary>
        public virtual string ToNormalizedString();

        /// <summary>
        /// Gives a full representation of the version including metadata.
        /// </summary>
        public virtual string ToFullString();
        public virtual string ToString(string format, IFormatProvider formatProvider);

        public override string ToString();
        public override int GetHashCode();

        public virtual SemanticVersion Clone();
        object ICloneable.Clone() => Clone();

        public virtual bool Equals(SemanticVersion other);
        public virtual bool Equals(SemanticVersion other, SemanticVersionComparison versionComparison);
        public virtual bool Equals(Version other);

        public virtual int CompareTo(object obj);
        public virtual int CompareTo(SemanticVersion other);
        public virtual int CompareTo(SemanticVersion other, SemanticVersionComparison versionComparison);
        public virtual int CompareTo(Version other);

        public static SemanticVersion Parse(string versionString);
        public static bool TryParse(string versionString, out SemanticVersion version);

        public static bool operator ==(SemanticVersion left, SemanticVersion right);
        public static bool operator !=(SemanticVersion left, SemanticVersion right);
        public static bool operator <(SemanticVersion left, SemanticVersion right);
        public static bool operator <=(SemanticVersion left, SemanticVersion right);
        public static bool operator >(SemanticVersion left, SemanticVersion right);
        public static bool operator >=(SemanticVersion left, SemanticVersion right);

        public static explicit operator SemanticVersion(Version version); 

        public static bool operator ==(SemanticVersion left, Version right);
        public static bool operator !=(SemanticVersion left, Version right);
        public static bool operator <(SemanticVersion left, Version right);
        public static bool operator <=(SemanticVersion left, Version right);
        public static bool operator >(SemanticVersion left, Version right);
        public static bool operator >=(SemanticVersion left, Version right);
    }

    public enum SemanticVersionComparison
    {
        /// <summary>
        /// Semantic version 2.0.1-rc comparison.
        /// </summary>
        Default = 0,
        /// <summary>
        /// Compares only the version numbers.
        /// </summary>
        Version = 1,
        /// <summary>
        /// Include Version number and Release labels in the compare.
        /// </summary>
        VersionRelease = 2,
        /// <summary>
        /// Include all metadata during the compare.
        /// </summary>
        VersionReleaseMetadata = 3
    }
}

clairernovotny commented 7 years ago

@NickCraver The public SemanticVersion Clone() method would need to be virtual based on the current pattern.

NickCraver commented 7 years ago

Oh yes, updating it that way.

I bet we end up sealed here, but I can't be the one to do it, @davidfowl won't talk to me again.

clairernovotny commented 7 years ago

I think the ReleaseLabels needs a way to be set from a derived class as well. Not sure if it should just be an IList<string> or be virtual?

yishaigalatzer commented 7 years ago

Please don't ever seal, that's just an unnecessary constraint.

Version ranges do not belong in dotnet core, they are a nuget feature that we plan to keep expanding. Either all of these go to a package from nuget we can keep revving without being tied to dotnet releases as we add new features or stick with just version.

Metadata should not be part of the comparison nor should imho the enum be there. If you want to compare the core version just get the version component out of it. Keep it simple.

Semver only supports 3 levels of version, but nuget always allowed 4 to match backward compatibility

A semver1 vs semver2 class or distinction is required.

vcsjones commented 7 years ago

One issue with the equality operators is this scenario:

Version version = new Version(1, 0, 0);
SemanticVersion semver = new SemanticVersion(1, 0, 0);

var equal2 = semver == version; //Compiles
var equal1 = version == semver; //Does not compile

I'm not a big fan of having equality operators having left and right be in a specific order, by type.

This can be solved in a few ways.

Implement the same operators on Version.

Implement the operators on SemanticVersion with left and right swapped, e.g.:

public static bool operator ==(SemanticVersion left, Version right);
public static bool operator ==(Version left, SemanticVersion right);
public static bool operator !=(SemanticVersion left, Version right);
public static bool operator !=(Version left, SemanticVersion right);
//etc...

Decide it is not a problem that needs to be solved.

Personally I like the idea of Version also having matching operators. I suppose there is probably precedent somewhere in the framework that should be followed for how this might be solved.

This also opens up the discussion that currently a SemanticVersion knows how to compare itself to a Version, but a Version doesn't know how to compare itself to a SemanticVersion.

I think it would make sense for that to all be a single proposal, otherwise it's difficult to see how all of the pieces fit together.

NickCraver commented 7 years ago

@yishaigalatzer Good thoughts, much appreciated.

Version ranges do not belong in dotnet core, they are a nuget feature that we plan to keep expanding. Either all of these go to a package from nuget we can keep revving without being tied to dotnet releases as we add new features or stick with just version.

I agree on ranges (and didn't include them). If they are only NuGet specific then yes, we should leave them out and hopefully leave this extensible where NuGet can use the base SemanticVersion, if that can be reasonably done. I'm not sure I follow on the second half. Many more people than NuGet have a need for SemanticVersion (hence this proposal), it should be more widely available that that, and IMO, a BCL include as Version is. I'm not 100% against it being in another library, e.g. System.SemanticVersioning, but can you provide some examples of how NuGet has evolved this?

A few followup questions:

A concern: will anyone find it? Or will we have many developers creating it? I hope tooling solves this and makes it a non-issue.
The arguments here are that some pieces are NuGet specific and have no place in a base version. Totally valid. What would be in this package then? A base version that NuGet can inherit in Nuget.Versioning? If so, have the bits in this base version revved any, or just the inheritor NuGet would maintain? I'm trying to ascertain the split of where the changes are happening.
Does the trend towards fewer packages in .NET 2.0 not go against this approach of smaller ones for everything? Maybe @terrajobst can comment on suggestions there?

Metadata should not be part of the comparison nor should imho the enum be there. If you want to compare the core version just get the version component out of it. Keep it simple.

After reading this, I agree. Thoughts from others on nuking the enum and overloads?

A semver1 vs semver2 class or distinction is required.

Perhaps a return of the version in enum form (which can be added to in a non-breaking way) would be the best option here? e.g.:

    public SemanticVersionSpecification VersionSpecification { get; };
...
public enum SemanticVersionSpecification {
    Version1,
    Version2
}

...these are terrible names, but you get the idea. We could determine the minimum (or maximum) level it's compatible with similar to how NuGet does this today.

@vcsjones I looked at the first example I could think of being added later: BigInterger, and it implements both directions, though it's in another library so I think it's viewable to do either way given the inclusion situation.

vcsjones commented 7 years ago

@NickCraver

I looked at the first example I could think of being added later: BigInterger, and it implements both directions

That seems the best way to go. The more I think on it, the more I dislike the idea of Version knowing anything about SemanticVersion. I would then suggest that the following be added to the proposal:

public static bool operator ==(Version left, SemanticVersion right);
public static bool operator !=(Version left, SemanticVersion right);
public static bool operator <(Version left, SemanticVersion right);
public static bool operator <=(Version left, SemanticVersion right);
public static bool operator >(Version left, SemanticVersion right);
public static bool operator >=(Version left, SemanticVersion right);

dasMulli commented 7 years ago

@NickCraver

I believe the metadata should not be a single string field but rather an IEnumerable<string> just like ReleaseLabels. The semver "spec" has specific requirements about how these have to be constructed so it would seem like a good place to put the logic and not require users to look up and implement it themselves.
Does the type have to be class or could it be a struct? That way, no Clone() is needed. I don't really see a benefit for using a class here since it is meant to be an immutable structure judging from the signatures. One would loose the ability to override the virtual members though (so it's just like sealed)
On the sealed topic: If i build an API with it, i would expect the behaviour to be the one defined by the BCL and not from a different user's subclass - having an overwritten CompareTo() on an instance passed to an API method, this can potentially blow up. Or a subclass that returns a different metadata string on every call because someone really tries to break my code :trollface: . I do believe that there are benefits for implementing custom comparisons but then the user should make use of custom comparers and be aware of the context of the custom logic. The only use case i can think of is when a new SemVer version is released and there is a class SemanticVersion3 : SemanticVersion. But that would break any expectation about the behaviour just as much as a custom subclasses would.

Addition: I would also very much like to see an AssemblySemanticVersionAttribute that makes use of the proposed SemanticVersion. Would be a piece of 🍰 to generate using the new msbuild system and be of great use for diagnostic / logging purposes.

jnm2 commented 7 years ago

I would also very much like to see an AssemblySemanticVersionAttribute that makes use of the proposed SemanticVersion. Would be a piece of 🍰 to generate using the new msbuild system and be of great use for diagnostic / logging purposes.

It would be super nice to be able to do this in one attribute instead of two:

[assembly: AssemblyVersion("2.0.10.0")]
[assembly: AssemblyInformationalVersion("2.0.10")] // Or sometimes "2.0.10-rc.1"

tjrobinson commented 7 years ago

This implementation of SemVer might be worth a look: https://github.com/AndyPook/SemVer There are some unit tests based on the spec.

sharwell commented 7 years ago

:question: Why is the operator to convert Version to SemanticVersion an explicit operator instead of implicit? I'm generally a fan of avoiding implicit operators, but this seems like a case where the conversion would always succeed without loss of information or broken behavior.

:question: Is there ever a case where semVer {operator} (SemanticVersion)ver is not the same as semVer {operator} ver (where ver is a Version)?

vcsjones commented 7 years ago

Another question:

SemVer does not have 4 components in its version number, it has major, minor, patch. However, this API proposal allows creating an instance from a Version, either by cast or through the constructor. There are also constructor overloads that accept revision. However, it isn't exposed as a property like the other components.

What is the purpose of this 4th component? Are we just ignoring it? If so, then @sharwell's suggestion of an implicit cast is a no-go since the 4th component is ignored. If we aren't ignoring it, are we OK deviating from the SemVer spec?

JonHanna commented 7 years ago

I'm not sure about the comparisons. Given 0.9, 0.9.1 and 1.0-beta, surely one always considers 1.0-beta the highest, but vary one whether or not one filters it out of consideration?

yishaigalatzer commented 7 years ago

Nuget respects all 4 due to historical reasons and hence it can't be dropped (at least in the nuget type)

From: Jon Hanna notifications@github.com Sent: Dec 2, 2016 11:32 AM To: dotnet/corefx Cc: Yishai Galatzer; Mention Subject: Re: [dotnet/corefx] Proposal: Adding System.SemanticVersion (#13526)

I'm not sure about the comparisons. Given 0.9, 0.9.1 and 1.0-beta, surely one always considers 1.0-beta the highest, but vary one whether or not one filters it out of consideration?

- You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdotnet%2Fcorefx%2Fissues%2F13526%23issuecomment-264541657&data=02%7C01%7Cyigalatz%40microsoft.com%7C35425a522d964b53c7ab08d41ae9e551%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636163039256770159&sdata=TpOf31p%2FFKiBgbDDucfbgrwSfJ7PnjPffBSPPdJ%2BGXQ%3D&reserved=0, or mute the threadhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABLmt9tG6qPjiygl6FsuOAz5yKC0zeLzks5rEHIygaJpZM4Kt7_i&data=02%7C01%7Cyigalatz%40microsoft.com%7C35425a522d964b53c7ab08d41ae9e551%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636163039256770159&sdata=Ok6ojCfSCQfi%2FaBVvyvL58dOpnFa2mMWsmbz2zDazfk%3D&reserved=0.

AlexGhiondea commented 7 years ago

@NickCraver I have updated your initial comment to contain the proposed API. Please take a look and let me know if it looks right.

karelz commented 7 years ago

@yishaigalatzer can you please review the API proposal as well?

karelz commented 7 years ago

@yishaigalatzer is OOF, he suggested to ask @rrelyea and @emgarten from NuGet team for the API review.

vcsjones commented 7 years ago

@karelz @AlexGhiondea I feel pretty strongly that these should also be part of the proposal: https://github.com/dotnet/corefx/issues/13526#issuecomment-260192413. Otherwise, the equality operators are not symmetric.

This is the same design as BigInteger as @NickCraver pointed out, and solves the problem where version == symanticVersion would not compile but symanticVersion == version would.

AlexGhiondea commented 7 years ago

@vcsjones agreed. I updated the comment at the top with those APIs.

karelz commented 7 years ago

@rrelyea, @emgarten did you get chance to review the API? We have BCL API review tomorrow morning and would like to make the call ideally.

emgarten commented 7 years ago

I would remove ToNormalizedString and ToFullString, they exist in NuGet for legacy reasons. NuGet allows versions with leading zeros (ex: 1.0.0001) and ToString returns it verbatim. For this class I would expect it to throw during parse if an invalid version were passed in, which would eliminate the need for ToNormalizedString. NuGet uses ToFullString to make the caller opt in to getting the build metadata, but for this new class I would expect it to be the default ToString behavior, ignoring metadata makes sense for NuGet, but I don't think other users would want that opinion.
Metadata as IEnumerable<string> makes sense as @dasMulli pointed out, it should be the same as ReleaseLabels, semver has similar rules for them. NuGet just didn't have a reason to read them individually.
The constructors with revision can be removed as @vcsjones said. In NuGet the SemanticVersion class holds onto this for the NuGetVersion class that extends it and compares with the 4th digit. Here it would never be used.
It is a bit unclear how a System.Version with a non-zero revision would be parsed here, would this throw as invalid?
A Release property that joins the release labels together might be helpful, but this could also be done with the format provider. Same for Metadata if it is moved to IEnumerable.
The 2.0.1-rc comparison mentioned in the comments refers to comparing release labels case-insensitively, which was never approved. For NuGet treating 1.0.0-BETA and 1.0.0-beta as completely different versions does not work well for URLs, and it causes problems when trying to extract them to disk on a case-insensitive file system. I think it is worth discussing what the behavior should be for System.SemanticVersion, and if it would be confusing to differ from NuGet.
Metadata and ReleaseLabels should be the same in terms of virtual or non-virtual.

NuGet could build on this class and expose the 4th digit for legacy. Much of the logic is in version comparer and formatter, if those could also be overridden in places to allow keeping the difficult release label comparing the same, but allowing extra comparing on the revision it would be perfect. 🎉

vcsjones commented 7 years ago

@karelz @emgarten Can we clarify if this proposal is going to have 4 components to cover NuGet's needs, since they do require 4 version components, or that this is supposed to be a true reflection of SemVer?

jnm2 commented 7 years ago

I'm holding my breath and hoping you'll do the best thing for the wider BCL consumer base, even if that isn't ideal for NuGet.

karelz commented 7 years ago

@NickCraver do you want to incorporate NuGet team feedback from @emgarten above?

emgarten commented 7 years ago

Can we clarify if this proposal is going to have 4 components to cover NuGet's needs, since they do require 4 version components, or that this is supposed to be a true reflection of SemVer?

I would expect System.SemanticVersion to be 3 parts and follow the SemVer rules. NuGet can continue supporting the legacy 4 part versions separately.

NickCraver commented 7 years ago

Here's a revised API based on feedback that mostly removes the NuGet specific pieces. I want to sanity check here before updating the top (since GitHub doesn't show history):

namespace System 
{
    public class SemanticVersion : IFormattable, IComparable, IComparable<SemanticVersion>, IEquatable<SemanticVersion>, ICloneable
    {
        public SemanticVersion(int major, int minor, int patch);
        public SemanticVersion(int major, int minor, int patch, string releaseLabel);
        public SemanticVersion(int major, int minor, int patch, string releaseLabel, string metadata);
        public SemanticVersion(int major, int minor, int patch, IEnumerable<string> releaseLabels, IEnumerable<string> metadata);
        public SemanticVersion(string version);

        protected SemanticVersion(Version version, string releaseLabel = null, string metadata = null);
        protected SemanticVersion(Version version, IEnumerable<string> releaseLabels, IEnumerable<string> metadata);

        public int Major { get; }
        public int Minor { get; }
        public int Patch { get; }
        public virtual IEnumerable<string> ReleaseLabels { get; }
        public virtual bool IsPrerelease { get; }
        public virtual IEnumerable<string> Metadata { get; }
        public virtual bool HasMetadata { get; }

        public virtual string ToString(string format, IFormatProvider formatProvider);

        public override string ToString();
        public override int GetHashCode();

        public virtual SemanticVersion Clone();
        object ICloneable.Clone() => Clone();

        public virtual bool Equals(SemanticVersion other);
        public virtual bool Equals(Version other);

        public virtual int CompareTo(object obj);
        public virtual int CompareTo(SemanticVersion other);
        public virtual int CompareTo(Version other);

        public static SemanticVersion Parse(string versionString);
        public static bool TryParse(string versionString, out SemanticVersion version);

        public static bool operator ==(SemanticVersion left, SemanticVersion right);
        public static bool operator !=(SemanticVersion left, SemanticVersion right);
        public static bool operator <(SemanticVersion left, SemanticVersion right);
        public static bool operator <=(SemanticVersion left, SemanticVersion right);
        public static bool operator >(SemanticVersion left, SemanticVersion right);
        public static bool operator >=(SemanticVersion left, SemanticVersion right);

        public static bool operator ==(Version left, SemanticVersion right);
        public static bool operator !=(Version left, SemanticVersion right);
        public static bool operator <(Version left, SemanticVersion right);
        public static bool operator <=(Version left, SemanticVersion right);
        public static bool operator >(Version left, SemanticVersion right);
        public static bool operator >=(Version left, SemanticVersion right);

        public static explicit operator SemanticVersion(Version version); 

        public static bool operator ==(SemanticVersion left, Version right);
        public static bool operator !=(SemanticVersion left, Version right);
        public static bool operator <(SemanticVersion left, Version right);
        public static bool operator <=(SemanticVersion left, Version right);
        public static bool operator >(SemanticVersion left, Version right);
        public static bool operator >=(SemanticVersion left, Version right);
    }
}

Open questions:

Are these the proper constructors?
- Note: I added a string version constructor, for parity with Version.
What does System.Version with a Revision > 0 do?
Should we add Metadata/Release properties which are string representations?
No one noticed I misspelled "Sysem"? Really?

AlexGhiondea commented 7 years ago

@NickCraver looks good!

I would not add the string representations for Metadata/Release -- do you have a use case in mind where that would be useful?

What do you mean by

What does System.Version with a Revision > 0 do?

I am curious about the protected method that has optional parameters on it -- is that needed in the implementation?

Worthaboutapig commented 7 years ago

I ended up writing my own SemanticVersion, so would love to see one in the BCL.

A question on HasMetadata; does it need to be virtual, any reason is can't simply be: public bool HasMetadata => Metadata != null; (?)

sharwell commented 7 years ago

@NickCraver

Simplifying interoperability with `Version`

I think the most important question is what to do with a System.Version where Revision > 0. If at all possible, I would want the answer to this question to have the following characteristic:

For all SemanticVersion s and Version v, and operator op in (==, !=, <, <=, >, >=):

The expression s op v is true if and only if s op (SemanticVersion)v is true
The expression v op s is true if and only if (SemanticVersion)v op s is true

:bulb: If the characteristic holds, I believe we can make the following changes:

 public virtual bool Equals(SemanticVersion other);
-public virtual bool Equals(Version other);

 public virtual int CompareTo(object obj);
 public virtual int CompareTo(SemanticVersion other);
-public virtual int CompareTo(Version other);

 public static SemanticVersion Parse(string versionString);
 public static bool TryParse(string versionString, out SemanticVersion version);

 public static bool operator ==(SemanticVersion left, SemanticVersion right);
 public static bool operator !=(SemanticVersion left, SemanticVersion right);
 public static bool operator <(SemanticVersion left, SemanticVersion right);
 public static bool operator <=(SemanticVersion left, SemanticVersion right);
 public static bool operator >(SemanticVersion left, SemanticVersion right);
 public static bool operator >=(SemanticVersion left, SemanticVersion right);

-public static bool operator ==(Version left, SemanticVersion right);
-public static bool operator !=(Version left, SemanticVersion right);
-public static bool operator <(Version left, SemanticVersion right);
-public static bool operator <=(Version left, SemanticVersion right);
-public static bool operator >(Version left, SemanticVersion right);
-public static bool operator >=(Version left, SemanticVersion right);

-public static explicit operator SemanticVersion(Version version); 
+public static implicit operator SemanticVersion(Version version); 

-public static bool operator ==(SemanticVersion left, Version right);
-public static bool operator !=(SemanticVersion left, Version right);
-public static bool operator <(SemanticVersion left, Version right);
-public static bool operator <=(SemanticVersion left, Version right);
-public static bool operator >(SemanticVersion left, Version right);
-public static bool operator >=(SemanticVersion left, Version right);

Inheritance

:bulb: At this point, I would recommend making the type sealed. In particular, I don't find the use case of attempting to layer NuGet's pseudo-semver handling on top of the proposed SemanticVersion class to be a compelling reason to make the new SemanticVersion class inheritable.

Cloning

:question: Why are we implementing ICloneable here?

:bulb: Since SemanticVersion is immutable, I would suggest removing the ICloneable interface and both Clone methods.

-public class SemanticVersion : IFormattable, IComparable, IComparable<SemanticVersion>, IEquatable<SemanticVersion>, ICloneable
+public class SemanticVersion : IFormattable, IComparable, IComparable<SemanticVersion>, IEquatable<SemanticVersion>
 {
     ...

-    public virtual SemanticVersion Clone();
-    object ICloneable.Clone() => Clone();

Labels

:question: Is there any reason not to provide access to a list of information instead of just an enumerable?

-public virtual IEnumerable<string> ReleaseLabels { get; }
+public virtual IReadOnlyList<string> ReleaseLabels { get; }
 public virtual bool IsPrerelease { get; }
-public virtual IEnumerable<string> Metadata { get; }
+public virtual IReadOnlyList<string> Metadata { get; }
 public virtual bool HasMetadata { get; }

NickCraver commented 7 years ago

@sharwell I'm still not sure how revision should behave, but any exceptional behavior is made far less clear with the implicit operator, IMO.

To address points specifically:

Implicit operator changes:
- On errors (re: Revision): it's not the comparison that failed, but a cast I didn't make. That's unintuitive for any use case. It also means we can't add any intellisense to those specific overloads telling the user why it'd blow up for the Revision cases (if that ends up being the behavior).
- It is, by necessity, an allocation of this SemanticVersion on every comparison. That's a pretty bad inefficiency.
- Inheritance: maybe so, I'd love for the NuGet team to chime in, and it's worth noting they may be one of potentially more use cases. SemVer has gone from 1.0 to 2.0, so the inheritable nature was more so with that in mind. That's a messy road in any situation, I don't see a clear path other than inheriting.
Cloning: it's mirroring System.Version here (and I assume: so is Nuget). I can't think of daily uses - perhaps someone can comment on System.Version's ICloneable history?
Labels: if we're sealing, I agree, read only collections as the accessor make sense. We'd need to make sure these aren't an allocation on access either and are backed as slim as possible.

NickCraver commented 7 years ago

@AlexGhiondea

What do you mean by "What does System.Version with a Revision > 0 do?"

The issue there is say we have a System.Version with a Revision of 6, what do we compare that to? It's basically a loss of precision issue since that 4th field is not a concept in semantic versioning.

I am curious about the protected method that has optional parameters on it -- is that needed in the implementation?

Depends if we seal, if it's sealed, that certainly changes.

@Worthaboutapig: Same story, depends on sealed or not. And the backer. I assume we'll likely store as a minimal ReadOnlyList<T> type on the backend, and assume null would be the case for no items passed to the constructor, yes. There's no reason to allocate if we have no members on the passed enumerable.

sharwell commented 7 years ago

@NickCraver

Implicit operator changes:
- "On errors" assumes errors. One way to convert Version to SemanticVersion is to use the format Major.Minor.Build+Revision. I believe this conversion also meets all of the conditions described in my previous comment.
- The allocation for operators is not likely to be a problem in practice. Typically people will be working with either Version or SemanticVersion, but not both. I would actually find it acceptable to keep the operator explicit and still remove all the other items, forcing the user to cast if they plan to use Version.

NickCraver commented 7 years ago

@sharwell While I agree that conversion may meet your conditions, it doesn't fit the scenario. You're effectively tossing away the revision precision (the root issue), or we decide to compare metadata as numbers for all comparisons...which also doesn't make much sense in a global approach.

As for allocations - we're shying away from that across the framework. I agree an explicit cast and API reduction is probably the happy medium there. Or, a constructor that takes Version simply being public...since that'd have to be the underlying implementation anyway.

AlexGhiondea commented 7 years ago

@sharwell if we just add Build and Revision you can get the wrong answer. Consider the case where you have 2 versions: 1.0.0.4 and 1.0.3.0 If you convert them to SemanticVersion you are going to get 1.0.4 and 1.0.3 which is not what you want.

If we want to normalize Build and Revision we can do something like Build*100 + Revision. To take the example from above, using this transformation you will get 1.0.4 and 1.0.300 which will maintain the relationship between the two versions.

The downside of this is that they are now looking rather ugly... and you might end up with an overflow when doing the transformation.

Stepping back, I think this type is going to be useful as a whole so I would like to move forward with a formal API review and discuss these few remaining open issues during the meeting. The behavior of the Revision != 0 conversion is an implementation detail and should not block progress on the API shape.

Looking at the API shape, the only think I am not sure of is the use of optional parameters.

@NickCraver - other than the behavior for Revision != 0 question is there anything else we need to figure out before we submit this for API review ?

NickCraver commented 7 years ago

@AlexGhiondea I think it's ready for review and most productive in a call. The Revision detail may affect API shape though (the necessary comparison operators to independently handle it may or may not be required). Let's see what a live discussion yields :)

AlexGhiondea commented 7 years ago

@NickCraver great! I have marked it as such!

Would you mind updating the top post with the most recent shape of the API to make it easy to find during the review?

dotnet / runtime