Standardize casing for the ClassicVersionWithTag

[andreasohlund]

Since ClassicVersionWithTag is supposed to be used to create nuget compatible versions we should standardize the casing of the "Beta" part. If we don't we can get in a situation where we have beta0001 and Beta0002. Because nuget is broken in terms of sorting this example will lead to beta0001 sorting higher. We just ran into this because GV defaults to beta with lowercase. When we then tagged the beta on GH with 5.0.0-Beta1 we then got a 5.0.0-Beta0002 from GV.

Proposal

I propose that we just lower case ClassicVersionWithTag always to make it simple (semver seems to be prefering lowercase).

This way you can tag with a "display friendly" tag without getting burned by nuget

[johnsimons]

I think we need this sooner then later, so is the fix to just lowercase the prerelease tag name ?

So my idea is to lowercase the name in the ctor of SemanticVersionPreReleaseTag, is this the way to go?

[andreasohlund]

@GeertvanHorrik was this on your radar? cc @JakeGinnivan

On Mon, Jul 14, 2014 at 8:32 AM, John Simons notifications@github.com
wrote:

I think we need this sooner then later, so is the fix to just lowercase
the prerelease tag name ?

So my idea is to lowercase the name in the ctor of
SemanticVersionPreReleaseTag, is this the way to go?

—
Reply to this email directly or view it on GitHub
#198 (comment)
.

[GeertvanHorrik]

No, wasn't aware of this one. I did to a ToLower for the NuGet versions, but not for the other ones.

[johnsimons]

So is my proposal the way to go?

[GeertvanHorrik]

Checking out my PR now to see if this is covered / can be easily covered.

[GeertvanHorrik]

I will add this code to SemanticVersionPreReleaseTag:

    public string Name
    {
        get { return _name; }
        set
        {
            if (!string.IsNullOrWhiteSpace(value))
            {
                value = value.ToLower();
            }

            _name = value;
        }
    }

[johnsimons]

I would just lower case it in the ctor:

public SemanticVersionPreReleaseTag(string name, int? number)
{
    Name = name.ToLowerInvariant();
    Number = number;
}

[GeertvanHorrik]

But then it can be set afterwards and screw things up?

[johnsimons]

yes u right, I missed that, I thought this class was immutable.

[SimonCropp]

FYI name is only changed in one location in NextSemverCalculator

[GeertvanHorrik]

I will keep it as property for now. An object is either immutable and should not have a public setter or must support all possible ways that it can be set, but that is just my opinion.

The new unit tests succeed for both ctor and setting it afterwards.

[JakeGinnivan]

Not sure changing in the SemanticVersion class is a good idea, why not just .ToLower in the ToString() method?

Reason being is we use it to read tags and compare, and we are then mutating what the tag actually is? Not sure if that is a problem, but something to consider

[andreasohlund]

Since we now havea NugetVersion should this perhaps only apply to that
variable?

Or should we always consider prerelease tags case insensitive no matter
what? (semver seem to use lcase but I haven't seen any statments regarding
if beta != Beta but I would assume that they should be treated equal)

On Mon, Jul 14, 2014 at 9:36 AM, Jake Ginnivan notifications@github.com
wrote:

Not sure changing in the SemanticVersion class is a good idea, why not
just .ToLower in the ToString() method?

Reason being is we use it to read tags and compare, and we are then
mutating what the tag actually is? Not sure if that is a problem, but
something to consider

—
Reply to this email directly or view it on GitHub
#198 (comment)
.

[GeertvanHorrik]

I think it should be consistent, so I vote for always lowercase.