Implement when PreleaseLabel is empty, the PreleaseTag is generated correctly

[HHobeck]

Fix [Bug] When PreleaseLabel is empty, the PreleaseTag is not correctly generated #2347

Close #2347

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

[HHobeck]

I'm done please review and merge to main.

[arturcic]

Is there any reason to have 4 new interfaces IContinuousDeliveryVersionCalculator, IContinuousDeploymentVersionCalculator, IManualDeploymentVersionCalculator, and ITrunkBasedVersionCalculator that have the same method with the same signature and to not share a IVersionCalculator?

If we take in consideration your suggestion regarding

public enum VersioningMode
{
    ManuallyDeployment, // previously ContinuousDelivery
    ContinuousDelivery, // previously ContinuousDeployment
    ContinuousDeployment, // new
    Trunkbased // previously Mainline??
}

I would rather have a IVersionCalculator that has the VersioningMode field and each implementation of this interfaces will set the appropriate VersioningMode andd then in the NextVersionCalculator I would inject all implementations and depending on the VersioningMode it will be selected the appropriate one. Thoughts?

[HHobeck]

Is there any reason to have 4 new interfaces IContinuousDeliveryVersionCalculator, IContinuousDeploymentVersionCalculator, IManualDeploymentVersionCalculator, and ITrunkBasedVersionCalculator that have the same method with the same signature and to not share a IVersionCalculator?

If we take in consideration your suggestion regarding

public enum VersioningMode
{
    ManuallyDeployment, // previously ContinuousDelivery
    ContinuousDelivery, // previously ContinuousDeployment
    ContinuousDeployment, // new
    Trunkbased // previously Mainline??
}

I would rather have a IVersionCalculator that has the VersioningMode field and each implementation of this interfaces will set the appropriate VersioningMode andd then in the NextVersionCalculator I would inject all implementations and depending on the VersioningMode it will be selected the appropriate one. Thoughts?

Yep good point. At this moment I have separated the behavior which not aligns with the VersioningMode enumerator. But if we are going to decide to change it so we have a one to one relation then yes I think your idea is great.

Do you have something like this in mind?

public interface IVersionCalculator
{
    VersioningMode VersioningMode { get; }

    SemanticVersion Calculate(NextVersion nextVersion);
}

public IVersionCalculator GetSpecificVersionCalculator(VersioningMode versioningMode)
{
    return _serviceProvider.GetServices<IVersionCalculator>().Single(
        element => element.VersioningMode == versioningMode
    );
}

[arturcic]

Is there any reason to have 4 new interfaces IContinuousDeliveryVersionCalculator, IContinuousDeploymentVersionCalculator, IManualDeploymentVersionCalculator, and ITrunkBasedVersionCalculator that have the same method with the same signature and to not share a IVersionCalculator?
If we take in consideration your suggestion regarding

public enum VersioningMode
{
    ManuallyDeployment, // previously ContinuousDelivery
    ContinuousDelivery, // previously ContinuousDeployment
    ContinuousDeployment, // new
    Trunkbased // previously Mainline??
}

I would rather have a IVersionCalculator that has the VersioningMode field and each implementation of this interfaces will set the appropriate VersioningMode andd then in the NextVersionCalculator I would inject all implementations and depending on the VersioningMode it will be selected the appropriate one. Thoughts?

Yep good point. At this moment I have separated the behavior which not aligns with the VersioningMode enumerator. But if we are going to decide to change it so we have a one to one relation then yes I think your idea is great.

Do you have something like this in mind?

public interface IVersionCalculator
{
    VersioningMode VersioningMode { get; }

    SemanticVersion Calculate(NextVersion nextVersion);
}

public IVersionCalculator GetSpecificVersionCalculator(VersioningMode versioningMode)
{
    return _serviceProvider.GetServices<IVersionCalculator>().Single(
        element => element.VersioningMode == versioningMode
    );
}

Exactly, in this case we don't need that many interfaces for the all the modes, and we have only the implementations and one interface

[arturcic]

Exactly, in this case we don't need that many interfaces for the all the modes, and we have only the implementations and one interface

So what do you think @HHobeck should we make those changes to the VersionMode, or put that change into another PR?

[asbjornu]

Exactly, in this case we don't need that many interfaces for the all the modes, and we have only the implementations and one interface

Agreed.

[HHobeck]

Exactly, in this case we don't need that many interfaces for the all the modes, and we have only the implementations and one interface

So what do you think @HHobeck should we make those changes to the VersionMode, or put that change into another PR?

I think it is better to do this in a separate PR. You mentioned already to do this after the beta.2 release. Which is fine with me.

[HHobeck]

Also we need to discuss the naming. IMO the naming of VersioningMode is missleading. Maybe a better name might be DeploymentMode. What do you think? In addition what do you like more Trunkbased or Mainline?

public enum DeploymentMode
{
    ManuallyDeployment, // previously ContinuousDelivery
    ContinuousDelivery, // previously ContinuousDeployment
    ContinuousDeployment, // new
    Trunkbased // previously Mainline??
}

[arturcic]

@asbjornu do you agree with this PR to be merged?

[asbjornu]

This is great stuff. The only thing I'm a bit wary about is the change from 1.0.1+46 to 1.0.1-46 unless label is set to null. Not because the change doesn't make sense, I think it's a good default, but because it's a breaking change. However, I think we should test this change out in the community and see what feedback we get.

[asbjornu]

Didn't we agree to reduce these interfaces down to just one, as they all have the same method signature SematicVersion Calculate(NextVersion nextVersion)?

[arturcic]

Well we discussed about that, but agreed to do the change after beta2, as it will require changing the VersioningMode

[asbjornu]

Can you please explain why merging these interfaces will require changing VersioningMode?

[arturcic]

Please check this changes

https://github.com/GitTools/GitVersion/pull/3447/files#diff-d1bb8310a1a6168b70d713420c6ecc455c3bd3311b4735c3811fb8e249db8da4R61-R69

(NextVersionCalculator.cs / CalculateIncrementedVersion)

We currently have 3 VersionModes but we have 4 interfaces, and @HHobeck proposed to change the VersionMode to

public enum VersioningMode
{
    ManuallyDeployment, // previously ContinuousDelivery
    ContinuousDelivery, // previously ContinuousDeployment
    ContinuousDeployment, // new
    Trunkbased // previously Mainline??
}

Or

public enum DeploymentMode
{
    ManuallyDeployment, // previously ContinuousDelivery
    ContinuousDelivery, // previously ContinuousDeployment
    ContinuousDeployment, // new
    Trunkbased // previously Mainline??
}

Then we have 4 modes and 4 interfaces. In that case if we define a property in a single interface like

public interface IVersionCalculator
{
    VersioningMode VersioningMode { get; }

    SemanticVersion Calculate(NextVersion nextVersion);
}

each implementation of this interface will set the mode accordingly.

[asbjornu]

I still don't understand why we can't have one interface with 4 different implementations? I would also prefer discussing renaming and other large changes in separate issues. If we are to keep a mode for what is basically mainline, I think we can just keep the name Mainline.

[HHobeck]

Mainline is confisuing IMO because we have a IsMainline property as a branch configuration and a Mainline value on the version mode. If you say the branch needs to be mainline then it's confusing. It was just a proposal from my side because if we are touching this lines of code it's easy to do it right away. But if you don't like it nevermind.

[asbjornu]

Just a couple of typos, then I think this is good.

[asbjornu]

Superb! 👏🏼

[mergify]

Thank you @HHobeck for your contribution!