Implements regular expression for determining version label

[CasperWSchmidt]

Description

• Remove magic string 'useBranchName'
• Merge branch configuration property BranchPrefixToTrim with Regex/RegularExpression to support dynamic named groups in the regular expression

Related Issue

#3661

Motivation and Context

See related issue.

How Has This Been Tested?

Existing tests are all green.

Screenshots (if appropriate):

N/A

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.

[CasperWSchmidt]

I've opened this as a draft as I'm not sure what documentation to update or extra tests to write (and where to put them).

Some guidance is welcome :)

[CasperWSchmidt]

Also, I'm not quite sure removing BranchPrefixToTrim is actually the better solution here. I'm no expert in Regex, but I don't see a way to capture the whole branch name while also having a prefix to match (fx. regex: ^features?[/-] to match all branches with the given prefix, but `{BranchName} should also contain the prefix, not just what comes next). This would be possible if BranchPrefixToTrim is kept by keeping that empty.

Or maybe I've just messed it up and used the branch-matching regex property instead of what was ment in the issue.

[HHobeck]

Also, I'm not quite sure removing BranchPrefixToTrim is actually the better solution here. I'm no expert in Regex, but I don't see a way to capture the whole branch name while also having a prefix to match (fx. regex: ^features?[/-] to match all branches with the given prefix, but `{BranchName} should also contain the prefix, not just what comes next). This would be possible if BranchPrefixToTrim is kept by keeping that empty.

Or maybe I've just messed it up and used the branch-matching regex property instead of what was ment in the issue.

Hi Casper.

Thank you for your PR. Could you please give an example in which circumstances you see problems? You have specified already the regular expression with e.g. ^features?[/-](?<BranchName>.+) on feature branch configuration. If you take a branch with name e.g. feature/foo the prefex feature/ will be trimmed and the branch name foo used as label.

[HHobeck]

I haven't found anything special. All comments from me are just minor issues. Well done.

[HHobeck]

I've opened this as a draft as I'm not sure what documentation to update or extra tests to write (and where to put them).

Some guidance is welcome :)

If you like you can create a unit test for:

• ConfigurationExtensions::GetBranchSpecificLabel

and some integration test in:

• src\GitVersion.Core.Tests\IntegrationTests\

If you not finding a test class which fits to your scenario please feel free and create a new one.

[CasperWSchmidt]

Also, I'm not quite sure removing BranchPrefixToTrim is actually the better solution here. I'm no expert in Regex, but I don't see a way to capture the whole branch name while also having a prefix to match (fx. regex: ^features?[/-] to match all branches with the given prefix, but `{BranchName} should also contain the prefix, not just what comes next). This would be possible if BranchPrefixToTrim is kept by keeping that empty.
Or maybe I've just messed it up and used the branch-matching regex property instead of what was ment in the issue.

Hi Casper.

Thank you for your PR. Could you please give an example in which circumstances you see problems? You have specified already the regular expression with e.g. ^features?[/-](?<BranchName>.+) on feature branch configuration. If you take a branch with name e.g. feature/foo the prefex feature/ will be trimmed and the branch name foo used as label.

Sure, As I said, I don't work much with Regex so I might just be too inexperienced, but let's say that you want branch configuration to match ^features?[/-], but you still want to include features as part of the {BranchName} token.

Can that be handled by using (?<BranchName>^features?[/-].+)? If so, I don't see any problems :)

[CasperWSchmidt]

I've opened this as a draft as I'm not sure what documentation to update or extra tests to write (and where to put them).
Some guidance is welcome :)

If you like you can create a unit test for:

• ConfigurationExtensions::GetBranchSpecificLabel

and some integration test in:

• src\GitVersion.Core.Tests\IntegrationTests\

If you not finding a test class which fits to your scenario please feel free and create a new one.

I've created a unit test for it, but I'm not really sure if an integration test makes sense. It's all about how the label is decided (which is covered by the unit test), not whether the label is added correctly to the final version strings. Other existing tests already takes care of that :)

[HHobeck]

Can that be handled by using (?<BranchName>^features?[/-].+)? If so, I don't see any problems :)

Yes exactly this is supported. That is the reason I have proposed to use capturing group because you can almost do everything. ;)

[HHobeck]

I've opened this as a draft as I'm not sure what documentation to update or extra tests to write (and where to put them).
Some guidance is welcome :)

If you like you can create a unit test for:

• ConfigurationExtensions::GetBranchSpecificLabel

and some integration test in:

• src\GitVersion.Core.Tests\IntegrationTests\

If you not finding a test class which fits to your scenario please feel free and create a new one.

I've created a unit test for it, but I'm not really sure if an integration test makes sense. It's all about how the label is decided (which is covered by the unit test), not whether the label is added correctly to the final version strings. Other existing tests already takes care of that :)

You can see the integration tests as a mechanism to ensure your functionality from the user point of view. It's like an acceptance test. It is really no big deal and you can create it very easily. Please give it a try and just wirte your scenario as part of an integration test. You are always sure your feature will not break.

Edit:
I would suggest you to write following integration test:

Given a feature branch configuration with

• RegularExpression ^features?[/-](?<TaskNumber>\d+)_(?<BranchName>.+)
• Label {BranchName}-of-task-number-{TaskNumber}

When getting the semantic version on feature branch feature/4711_this-is-a-feature
Then the pre-release label of calculated version is this-is-a-feature-of-task-number-4711

[HHobeck]

If you not mind could you please change the .* expression to .+ in the RegularExpression and create one integration test to ensure this functionality? In advance I see some tests who are not running successfully. Could you take a look as well?

After this changes we can merge it to the main branch. Thank you very much for your ambition.

[CasperWSchmidt]

I've opened this as a draft as I'm not sure what documentation to update or extra tests to write (and where to put them).
Some guidance is welcome :)

If you like you can create a unit test for:

• ConfigurationExtensions::GetBranchSpecificLabel

and some integration test in:

• src\GitVersion.Core.Tests\IntegrationTests\

If you not finding a test class which fits to your scenario please feel free and create a new one.

I've created a unit test for it, but I'm not really sure if an integration test makes sense. It's all about how the label is decided (which is covered by the unit test), not whether the label is added correctly to the final version strings. Other existing tests already takes care of that :)

You can see the integration tests as a mechanism to ensure your functionality from the user point of view. It's like an acceptance test. It is really no big deal and you can create it very easily. Please give it a try and just wirte your scenario as part of an integration test. You are always sure your feature will not break.

Edit: I would suggest you to write following integration test:

Given a feature branch configuration with

• RegularExpression ^features?[/-](?<TaskNumber>\d+)_(?<BranchName>.+)
• Label {BranchName}-of-task-number-{TaskNumber}

When getting the semantic version on feature branch feature/4711_this-is-a-feature Then the pre-release label of calculated version is this-is-a-feature-of-task-number-4711

I've tried to implement the example you've given but have run into an issue. In GetBranchSpecificLabel the branch name is sanitized using effectiveBranchName = effectiveBranchName.RegexReplace("[^a-zA-Z0-9-]", "-");. This changes the branch name from feature/4711_this-is-a-feature to feature/4711-this-is-a-feature (notice _ is changed to -) which messes up the ability to match the regular expression using _. Changing the regular expression to use - instead doesn't work either because the branch name (without sanitizing) no longer match and so the unknown branch configuration is used as fallback.

The RegexReplace is something I've copied from the existing code so I'm hesitant to modify it without consulting you first. Adding _ to the RegexReplace valid characters fixes this but I'm not sure if this is something you want? TBH I don't quite understand why underscore is replaced, but adding it doesn't break any tests.

[CasperWSchmidt]

If you not mind could you please change the .* expression to .+ in the RegularExpression and create one integration test to ensure this functionality? In advance I see some tests who are not running successfully. Could you take a look as well?

After this changes we can merge it to the main branch. Thank you very much for your ambition.

I changed the expression this morning (CET) so that should be ok. I've also fixed the tests now.

[arturcic]

@HHobeck please consider merging this, good stuff

[mergify]

Thank you @CasperWSchmidt for your contribution!