Change GitVersionConfiguration and BranchConfiguration to record type and extract interfaces #3433

HHobeck · 2023-03-10T19:14:50Z

Change GitVersionConfiguration and BranchConfiguration to record type and extract interfaces

Description

Preparation for #3306

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.

… and extract interfaces.

arturcic · 2023-03-10T19:36:55Z

src/GitVersion.Core/Core/RepositoryStore.cs

@@ -104,8 +104,9 @@ public IBranch GetTargetBranch(string? targetBranchName)

    public IBranch? FindMainBranch(GitVersionConfiguration configuration)
    {
+        var branches = configuration.Branches;
        var mainBranchRegex = configuration.Branches[ConfigurationConstants.MainBranchKey].Regex


Suggested change

var mainBranchRegex = configuration.Branches[ConfigurationConstants.MainBranchKey].Regex

var mainBranchRegex = branches[ConfigurationConstants.MainBranchKey].Regex

arturcic · 2023-03-10T19:44:56Z

src/GitVersion.Core/Configuration/BranchConfiguration.cs

-    [JsonPropertyDescription("The regex pattern to use to match this branch.")]
-    public string? Regex { get; set; }
+    [JsonPropertyDescription("The regular expression pattern to use to match this branch.")]
+    public string? Regex { get; internal set; }


Suggested change

public string? Regex { get; internal set; }

public string? RegularExpression { get; internal set; }

arturcic · 2023-03-10T19:47:10Z

src/GitVersion.Core/Configuration/GitVersionConfiguration.cs

+    public SemanticVersionFormat SemanticVersionFormat { get; internal set; }
+
+    [JsonIgnore]
+    IReadOnlyDictionary<string, BranchConfiguration> IGitVersionConfiguration.Branches => Branches;


I wonder if we can use the newly interface here

Suggested change

IReadOnlyDictionary<string, BranchConfiguration> IGitVersionConfiguration.Branches => Branches;

IReadOnlyDictionary<string, IBranchConfiguration> IGitVersionConfiguration.Branches => Branches;

and the next line as well?

Only the first line because the second line is used for initialiization.

arturcic · 2023-03-10T19:48:47Z

@HHobeck will this follow up with a PR that will start using the new interfaces in the core instead of the classes? Or we can include that in the current PR?

arturcic · 2023-03-10T20:00:21Z

Please also run from the root

dotnet run --project .\src\GitVersion.Schema --framework net7.0 -- --outputdirectory schemas --version 6.0

to generate the updated schema as you've changed the descriptions

…sible. Execute: dotnet run --project .\src\GitVersion.Schema --framework net7.0 -- --outputdirectory schemas --version 6.0

…on where possible.

HHobeck · 2023-03-10T22:18:53Z

@HHobeck will this follow up with a PR that will start using the new interfaces in the core instead of the classes? Or we can include that in the current PR?

I think it might be better to separate it and create another PR.

src/GitVersion.Core/Configuration/IIgnoreConfiguration.cs

arturcic · 2023-03-11T02:53:10Z

TODOs for the next PR:

make IgnoreConfiguration, BranchConfiguration and GitVersionConfiguration records internal
make usage of the respective interfaces in the code (maybe with the exception of unit tests)
consider making the properties init instead of private set for the above records, and maybe readonly were possible
consider moving the [JsonProperty*] attributes from the record to the interface and adapt the schema generation to use the interfaces instead

Please add more to this list what you have in mind, and then you can use this list as description for the new PR

arturcic · 2023-03-11T02:56:42Z

LGTM! It moves in the direction that will allow us to move the implementations details of Configuration into its own module

…rd, to an extension at the place of usage

HHobeck · 2023-03-11T07:34:13Z

TODOs for the next PR:

make IgnoreConfiguration, BranchConfiguration and GitVersionConfiguration records internal

make usage of the respective interfaces in the code (maybe with the exception of unit tests)

consider making the properties init instead of private set for the above records, and maybe readonly were possible

consider moving the [JsonProperty*] attributes from the record to the interface and adapt the schema generation to use the interfaces instead

Beside the fact that it might be a lot of work and tricky doing it I think it is not feasible (last point) moving the JsonProperty to the interface. The reason is that we are using interfaces and mutable data types in the interface and YamlDotnet cannot handle it.

LGTM! It moves in the direction that will allow us to move the implementations details of Configuration into its own module

Yes that's the goal to seperate the aspect of loading/merging the configuration from using it. I'm done please merge this PR to main.

mergify · 2023-03-11T08:00:06Z

Thank you @HHobeck for your contribution!

arturcic · 2023-03-11T10:05:48Z

Beside the fact that it might be a lot of work and tricky doing it I think it is not feasible (last point) moving the JsonProperty to the interface. The reason is that we are using interfaces and mutable data types in the interface and YamlDotnet cannot handle it.

That's the reason I put consider. I will have a look if it's feasible

Change GitVersionConfiguration and BranchConfiguration to record type…

6da7059

… and extract interfaces.

arturcic reviewed Mar 10, 2023

View reviewed changes

HHobeck added 2 commits March 10, 2023 22:27

Change usage of BranchConfiguration to IBranchConfiguration where pos…

95075cb

…sible. Execute: dotnet run --project .\src\GitVersion.Schema --framework net7.0 -- --outputdirectory schemas --version 6.0

Change the usage of GitVersionConfiguration to IGitVersionConfigurati…

af4eeea

…on where possible.

Fix ShouldFilterVersion unit test

b82e2cd

arturcic reviewed Mar 11, 2023

View reviewed changes

src/GitVersion.Core/Configuration/IIgnoreConfiguration.cs Outdated Show resolved Hide resolved

arturcic approved these changes Mar 11, 2023

View reviewed changes

Removed unnecessary blank line

ad1cc78

Move IIgnoreConfiguration::ToFilters method out of the interface/reco…

b6b9d94

…rd, to an extension at the place of usage

dotnet-format ./src/ --exclude **/AddFormats/

642a7da

arturcic enabled auto-merge March 11, 2023 07:48

arturcic merged commit 506b6ec into GitTools:main Mar 11, 2023

arturcic deleted the feature/change-configuration-to-record-type-and-extract-interfaces branch March 11, 2023 08:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change GitVersionConfiguration and BranchConfiguration to record type and extract interfaces #3433

Change GitVersionConfiguration and BranchConfiguration to record type and extract interfaces #3433

HHobeck commented Mar 10, 2023

arturcic Mar 10, 2023

HHobeck Mar 10, 2023

arturcic Mar 10, 2023

HHobeck Mar 10, 2023

arturcic Mar 10, 2023

HHobeck Mar 10, 2023 •

edited

Loading

arturcic commented Mar 10, 2023

arturcic commented Mar 10, 2023

HHobeck commented Mar 10, 2023 •

edited

Loading

arturcic commented Mar 11, 2023 •

edited

Loading

arturcic commented Mar 11, 2023

HHobeck commented Mar 11, 2023 •

edited

Loading

mergify bot commented Mar 11, 2023

arturcic commented Mar 11, 2023

	var mainBranchRegex = configuration.Branches[ConfigurationConstants.MainBranchKey].Regex
	var mainBranchRegex = branches[ConfigurationConstants.MainBranchKey].Regex

	public string? Regex { get; internal set; }
	public string? RegularExpression { get; internal set; }

	IReadOnlyDictionary<string, BranchConfiguration> IGitVersionConfiguration.Branches => Branches;
	IReadOnlyDictionary<string, IBranchConfiguration> IGitVersionConfiguration.Branches => Branches;

Change GitVersionConfiguration and BranchConfiguration to record type and extract interfaces #3433

Change GitVersionConfiguration and BranchConfiguration to record type and extract interfaces #3433

Conversation

HHobeck commented Mar 10, 2023

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Checklist:

arturcic Mar 10, 2023

Choose a reason for hiding this comment

HHobeck Mar 10, 2023

Choose a reason for hiding this comment

arturcic Mar 10, 2023

Choose a reason for hiding this comment

HHobeck Mar 10, 2023

Choose a reason for hiding this comment

arturcic Mar 10, 2023

Choose a reason for hiding this comment

HHobeck Mar 10, 2023 • edited Loading

Choose a reason for hiding this comment

arturcic commented Mar 10, 2023

arturcic commented Mar 10, 2023

HHobeck commented Mar 10, 2023 • edited Loading

arturcic commented Mar 11, 2023 • edited Loading

arturcic commented Mar 11, 2023

HHobeck commented Mar 11, 2023 • edited Loading

mergify bot commented Mar 11, 2023

arturcic commented Mar 11, 2023

HHobeck Mar 10, 2023 •

edited

Loading

HHobeck commented Mar 10, 2023 •

edited

Loading

arturcic commented Mar 11, 2023 •

edited

Loading

HHobeck commented Mar 11, 2023 •

edited

Loading