Magic strings >> BranchConfigurationKey

[9swampy]

We still use master in almost all repos and I've been struggling to get a working config in v6 so I started looking at the test suite.

Quite soon I came across many "main" magic strings but also this:

GitVersion/src/GitVersion.Core.Tests/Helpers/TestBase.cs

Line 10 in f8a95ab

public const string MainBranch = "main";

I reckoned if I changed that to master the generated test scenarios would behave accordingly, initializing and working with "master" instead of "main". But no, that constant is very sparingly used; many many magic strings all over and trying to make the tests behave quickly became a very noisy PR; so I just narrowed in on the absolute minimum so I could back this part out.

In the meantime I also encountered this (there's a whole suite of keys here):

GitVersion/src/GitVersion.Core/Configuration/ConfigurationConstants.cs

Line 31 in f8a95ab

public const string MainBranchKey = "main";

The concept of BranchKey aligns to the standard sections the GitVersion.yml anticipates.

Throughout the test suite both "classes" of constant are passed around as strings and it gets really confusing very quickly which is correct and which won't work in various places in the code.

Could I suggest introducing a clear split, making the ConfigurationBranchKey a type safe concept that can be used to tidy up the string passing methods and parameters while leaving the Tester free to choose whatever actual branch names they want for commits to the repo.

Something like this perhaps (yes I've gone quite far down the rabbit hole already but because the magic strings are pervasive and widespread the PR will be big (~130 files) but very focused and imho fairly easy to delta). Test coverage is obviously good because this is all about tests, barely impacts the core functional code at all. High confidence that I can fit this in without breaking or significantly altering any existing tests (incl. TestCaseAttributes, Branches[]/SourceBranches, BranchMetaData, Custom, Unknown & Other).

namespace GitVersion.Configuration;

public class BranchConfigurationKey : IEquatable<BranchConfigurationKey>
{
    [Obsolete("Use MainBranchKey instead.")]
    public static readonly BranchConfigurationKey Master = new(ConfigurationConstants.MasterBranchKey);
    public static readonly BranchConfigurationKey Main = new(ConfigurationConstants.MainBranchKey);
    public static readonly BranchConfigurationKey Develop = new(ConfigurationConstants.DevelopBranchKey);
    public static readonly BranchConfigurationKey Release = new(ConfigurationConstants.ReleaseBranchKey);
    public static readonly BranchConfigurationKey Feature = new(ConfigurationConstants.FeatureBranchKey);
    public static readonly BranchConfigurationKey PullRequest = new(ConfigurationConstants.PullRequestBranchKey);
    public static readonly BranchConfigurationKey Hotfix = new(ConfigurationConstants.HotfixBranchKey);
    public static readonly BranchConfigurationKey Support = new(ConfigurationConstants.SupportBranchKey);

    public string Value { get; }

    protected BranchConfigurationKey(string value)
    {
        if (string.IsNullOrEmpty(value))
            throw new ArgumentException("Value cannot be null or empty.", nameof(value));
        Value = value;
    }

    public override string ToString() => Value;
}

public class CustomBranchConfigurationKey : BranchConfigurationKey
{
    private static readonly HashSet<string> ReservedKeys = new(StringComparer.Ordinal)
    {
        ConfigurationConstants.MasterBranchKey,
        ConfigurationConstants.MainBranchKey,
        ConfigurationConstants.DevelopBranchKey,
        ConfigurationConstants.ReleaseBranchKey,
        ConfigurationConstants.FeatureBranchKey,
        ConfigurationConstants.PullRequestBranchKey,
        ConfigurationConstants.HotfixBranchKey,
        ConfigurationConstants.SupportBranchKey,
    };

    public CustomBranchConfigurationKey(string value) : base(value)
    {
        if (value is null)
            throw new ArgumentNullException(nameof(value), "Custom branch key cannot be null.");
        if (ReservedKeys.Contains(value))
            throw new ArgumentException($"'{value}' is a reserved branch configuration key and cannot be used for custom branches.", nameof(value));
    }
}

[HHobeck]

May I ask you: Why do you care about how the branch is named in the unit tests? Does it make any different if it is named master or main? From the configuration point of view both are included and treated the same. At the end everyone can use their own name and their own configuration in the unit tests. There is no need for unification IMO.

[9swampy]

Why do you care about how the branch is named in the unit tests? I don't.

I didn't know if using "master" branch which should regex back to "main" config was the reason why I've really struggled to get a config that works in v6.

When I started looking in to why I couldn't get a config that works the test code was extremely confusing. When you're talking about the config it does matter; so when you're passing around strings for both config ("main") and branch ("master" - which regexes back to the "main" config) it's not at all clear which to use in different scenarios.

The proposal is intended to address that confusion in tests; it doesn't adapt the core functionality at all. In real use you can continue to use whatever names you like for config and for branches.

I would propose that people writing tests have faced a similar challenge which ended up introducing "unknown", "other" and "custom" branch configuration keys. "Other" and "custom" I believe are exactly the same concept. "Unknown" is a different, test specific concept that's important to document clearly too.

The "Custom" configuration key is retained; and can be used as many times with any names the tester wants - so long as there isn't a name conflict.

Branch names remain as simple strings; but at least it's clear that it's a branch name, not a key to a configuration section.

[HHobeck]

It's quite simple we can delete all master branch values and use main instead. I'm surprised that even master is used. Master regex is only there for backwards compatibility reasons. Unknown branch configuration section is actual a reserved name and will be always treated as last element in the list. Of course you need to aware of what workflow you are extending (in the unit tests and in production). Therefor we have the ConfigurationBuilder classes in the unit tests:

• GitFlowConfigruationBuilder,
• GitHubFlowConfigurationBuilder,
• TrunkBasedConfigurationBuilder and
• EmptyConfigurationBuilder (from scratch)

And in production you can use the workflow property:

• GitFlow/v1,
• GitHubFlow/v1
• TrunkBased/preview1 and
• empty string (from scratch)

We are changing the workflow property in the upcoming major release (7.x) of gitversion and define it as obligatory. So it is clear what workflow (base template) is used. At the moment if you don't specify this property the default value of GitFlow/v1 will be assumed.

namespace GitVersion.Configuration;

public class BranchConfigurationKey : IEquatable<BranchConfigurationKey>
{
    [Obsolete("Use MainBranchKey instead.")]
    public static readonly BranchConfigurationKey Master = new(ConfigurationConstants.MasterBranchKey);

Master BranchConfigurationKey makes absolutely no sense in my opinion.

public class CustomBranchConfigurationKey : BranchConfigurationKey
{
    private static readonly HashSet<string> ReservedKeys = new(StringComparer.Ordinal)
    {
        ConfigurationConstants.MasterBranchKey,
        ConfigurationConstants.MainBranchKey,
        ConfigurationConstants.DevelopBranchKey,
        ConfigurationConstants.ReleaseBranchKey,
        ConfigurationConstants.FeatureBranchKey,
        ConfigurationConstants.PullRequestBranchKey,
        ConfigurationConstants.HotfixBranchKey,
        ConfigurationConstants.SupportBranchKey,
    };

    public CustomBranchConfigurationKey(string value) : base(value)
    {
        if (value is null)
            throw new ArgumentNullException(nameof(value), "Custom branch key cannot be null.");
        if (ReservedKeys.Contains(value))
            throw new ArgumentException($"'{value}' is a reserved branch configuration key and cannot be used for custom branches.", nameof(value));
    }
}

I'm also not sure what your intension is with introducing CustomBranchConfigurationKey class. Maybe you want to protect the user that he is using a reserved configuration section. This makes no sense because sometimes you want to overwrite some properties for e.g. main in you GitVersion.yml configuration. Also it is depends on what base template you are using (e.g. develop is not known in the GitHubFlow workflow).

The only improvement I can think of is to change the order of how the branch configuration section will be interpreted to match against the regular expression. We could say to first use the custom configuration section then the base configuration section (if they are named differently). In this case your configuration section with name master will be used before the main e.g. defined in the GitHubFlow/v1 base template. If not all mandatory fields are specified in a configuration section an exception should be thrown.

Does this make sense to you?

[9swampy]

Master BranchConfigurationKey makes absolutely no sense in my opinion.

Agreed, I'd only included it because it's present in the ConfigurationConstants on main. I did mark it obsolete too.

Consider the following test:

New to the codebase and wanting to replicate our usecase of the master branch default-mapping to the main configuration section

1. is not the branch name, it's the name of the configuration section - so I should leave it as "main"
2. is the actual branch name, so I should change it to "master"
3. is the actual branch name, so I should change it to "master"

Or here:

In both cases here we really are talking Actual.MainBranchName.

I'm also not sure what your intension is with introducing CustomBranchConfigurationKey class. Maybe you want to protect the user that he is using a reserved configuration section. This makes no sense because sometimes you want to overwrite some properties for e.g. main in you GitVersion.yml configuration. Also it is depends on what base template you are using (e.g. develop is not known in the GitHubFlow workflow).

When working through tests I appreciated that some tests stepped outside the ConfigurationConstants.[ReservedName]Keys list, so wanted to show that I'd considered the need to keep everything unrestricted. None of this was intended to restrict the user, merely to clarify the intent of tests. I'm a big fan of the builder pattern and that's precisely where I would have proposed this to fit in.

Maybe I've missed the {BranchNamePlaceholder} extraction.

Ultimately it was merely a proposal borne out of my experience new to the code base. I appreciate the time you took to respond with a detailed reply. Most makes perfect sense - trying to use the builders in the unit tests to help me build up a configuration that actually works with unit tests proofing the theory is exactly where I have been headed. I've a related PR documenting 2 problems encountered where maybe I need to better understand; but also where there's inconsistent behaviour between the test fixture behaviour and the published dotnet-gitversion cli. More to come. Unknown - I'd not fully comprehended but I followed the explanation; I'll revisit, thanks.

If you still don't see the value add and you want to keep strings for both branch and configuration key concepts, at the very least maybe you would consider, in tests, substituting some of the most confusing occurrences with named constants that would go a long way towards clarifying intent? After all the BranchConfigurationKey proposed wasn't much more than a type-safe wrapper around the ConfigurationConstants.[ReservedName]Keys anyhow.

[HHobeck]

I would agree on your clean code initiative by creating the method WithCustomBranchConfiguration and introducing type safety. In my opinion you need to be consequent and build a struct of type BranchKey and BranchName to encapsulate the aspects you mentioned. I would not agree with Actual.MainBranchName because it introduces another abstraction. At the end personally I would leave the branch key and branch name as it is, because it is not worth the effort. For me (and probably for the most user) the different between branch key and name is clear.

Anyway, improving code in the direction of readability (clean code programming) is something I would never stand in the way:

public record struct BranchKey
{
    private readonly string? value;

    public static BranchKey Empty => default;

    public string Value => value ?? string.Empty;

    public BranchKey(string value)
    {
        ArgumentException.ThrowIfNullOrEmpty(value);
        if (!value.All(char.IsLetter))
        {
            throw new ArgumentException("The parameter contains invalid character.", nameof(value));
        }
        this.value = value;
    }

    public static BranchKey From(string value) => new BranchKey(value);

    public override string ToString() => Value;

    public override int GetHashCode() => Value.GetHashCode();
}

[HHobeck]

This would be something which give the user a real benifit on usage:

The only improvement I can think of is to change the order of how the branch configuration section will be interpreted to match against the regular expression. We could say to first use the custom configuration section then the base configuration section (if they are named differently). In this case your configuration section with name master will be used before the main e.g. defined in the GitHubFlow/v1 base template. If not all mandatory fields are specified in a configuration section an exception should be thrown.

[9swampy]

I hadn't intended a master config as well as a main. Main section with the main|master regex. I'll bear in mind but for starters give branch key struct another go. I had discounted because couldn't use struct as test attribute parameters; had to keep the consts and the class wrapper I felt was justifiable. Let me give struct another go & revert.

Actual.MainBranchName is just a straight up magic string const, but fine BranchKey would sufficiently distinguish the contexts. Good enough and would focus the change.

Thx for the consideration

[HHobeck]

Maybe create an implicit cast operator from string to BranchName to keep the code change small.