Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor next version calculator (get rid of taggedSemanticVersion) #3244

Merged
merged 11 commits into from
Feb 19, 2023

Conversation

AlexPykavy
Copy link
Contributor

@AlexPykavy AlexPykavy commented Oct 24, 2022

Suggest to review by commits :) I tried to make them as atomic as possible.

Description

Minor changes:

  • Rearranging code
  • Renaming variables
  • Reusing existing functionality

Change next version calculator to act according to the next algorithm:

  1. Calculate next version candidate based on the base one
  2. Look for the latest PreReleaseTag matching Major, Minor and Patch of the next version candidate.
  3. Append the PreReleaseTag, if found, to the next version candidate.
  4. Compare the next version candidate with Context.CurrentCommitTaggedVersion including the PreReleaseTag comparison.
  5. Increment the PreReleaseTag of the next version candidate if versions mismatch.
  6. Return the next version candidate.

Related Issue

Motivation and Context

I found NextVersionCalculator logic complicated and decided to simplify it.

How Has This Been Tested?

By running all existing tests.

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
Copy link
Contributor

HHobeck commented Oct 28, 2022

I like your refactoring Alex. 👍 Good work. When I execute the unit tests I got some NullReferenceExceptions.

@AlexPykavy AlexPykavy force-pushed the refactor-next-version-calculator branch from c95b521 to 059375f Compare October 29, 2022 21:11
@AlexPykavy
Copy link
Contributor Author

I like your refactoring Alex. 👍 Good work. When I execute the unit tests I got some NullReferenceExceptions.

I fixed failing unit tests and they pass locally but I still have issues with some CI jobs:

  • FindsVersionInDynamicRepo("GV_main","https://github.com/GitTools/GitVersion","main","efddf2f92c539a9c27f1904d952dcab8fb955f0e","5.8.2+56") test fails on MacOS and it looks like some connectivity issue SecureTransport error: connection closed via error.
  • Also there are many failing jobs which test the artifact using .NET 7 SDK on Arm64. The majority of them are caused by MSB4018 error. I suspect that it can be the issue of the .NET itself because 7.0.100-rc.2.22477.23 version used for tests which may contain issues. Also, found an issue .Net 7 preview 7 is broken on linux arm64 via QEMU dotnet/sdk#27190 that may be related.

@AlexPykavy AlexPykavy force-pushed the refactor-next-version-calculator branch from 0977561 to c718ee0 Compare November 24, 2022 17:49
@AlexPykavy
Copy link
Contributor Author

@HHobeck , all failing tests are due to the .NET 7 instability on ARM. Can this PR be merged or it needs some corrections?

@arturcic arturcic force-pushed the refactor-next-version-calculator branch from c718ee0 to e7e7be5 Compare November 29, 2022 18:34
@arturcic
Copy link
Member

@AlexPykavy - can you please fix the warnings as well? https://github.com/GitTools/GitVersion/actions/runs/3577000238

@AlexPykavy AlexPykavy force-pushed the refactor-next-version-calculator branch from e7e7be5 to 20a8fe7 Compare November 29, 2022 19:27
@AlexPykavy
Copy link
Contributor Author

@AlexPykavy - can you please fix the warnings as well? https://github.com/GitTools/GitVersion/actions/runs/3577000238

Fixed, but I'm not sure about adding Context.CurrentCommitTaggedVersion != null before calling semver.CompareTo(Context.CurrentCommitTaggedVersion) because the method handles null arguments. Maybe it would be better to use null-forgiving operator to suppress the warning?

@HHobeck
Copy link
Contributor

HHobeck commented Nov 30, 2022

@AlexPykavy - can you please fix the warnings as well? https://github.com/GitTools/GitVersion/actions/runs/3577000238

Fixed, but I'm not sure about adding Context.CurrentCommitTaggedVersion != null before calling semver.CompareTo(Context.CurrentCommitTaggedVersion) because the method handles null arguments. Maybe it would be better to use null-forgiving operator to suppress the warning?

This might be also possible:

if (Context.CurrentCommitTaggedVersion?.CompareTo(semver) == 0)
{
...
}

@AlexPykavy AlexPykavy force-pushed the refactor-next-version-calculator branch from 20a8fe7 to 0e6554e Compare December 11, 2022 15:06
@AlexPykavy AlexPykavy force-pushed the refactor-next-version-calculator branch from 4a3d092 to e9edd32 Compare January 6, 2023 15:31
@HHobeck
Copy link
Contributor

HHobeck commented Jan 6, 2023

@arturcic: May I ask you is it possible to go away from .net standard? It makes no sense in my opinion if we move to .net 6 and 7 or I'm wrong? It would be nice to use the Init Only Setters feature.

@arturcic
Copy link
Member

arturcic commented Jan 6, 2023

Honestly I was considering that. We need to check the libgit2sharp if can pe used in this case

@HHobeck
Copy link
Contributor

HHobeck commented Jan 6, 2023

Honestly I was considering that. We need to check the libgit2sharp if can pe used in this case

But libgit2sharp is only referenced in GitVersion.App.csproj (Target Framework net6.0;net7.0) and not in GitVersion.Core.csproj (Target Framework .NET Standard 2.0)

Forget my post. I haven't seen the indirect dependency via GitVersion.LibGit2Sharp project

@HHobeck
Copy link
Contributor

HHobeck commented Jan 7, 2023

@AlexPykavy Could you please take a look on all references of the property PreReleaseTag and BuildMetaData and remove the following null checks?

  • e.g. PreReleaseTag?.HasTag(), PreReleaseTag?.Number, PreReleaseTag == null and PreReleaseTag.ShouldNotBeNull()
  • e.g. BuildMetaData?.Sha, BuildMetaData == null and PreReleaseTag.ShouldNotBeNull()

@HHobeck
Copy link
Contributor

HHobeck commented Jan 7, 2023

From my point of view the next refactoring steps are the following:

  • removing the .net standard target framework and change it to .net 6.0 and .net 7.0
  • fix all warnings because of the nullable feature (which is enabled by default)
  • change the SemanticVersionPreReleaseTag and SemanticVersionBuildMetaData to a struct data type and make it immutable
  • change the properties in SemanticVersionPreReleaseTag and SemanticVersionBuildMetaData to read only using Init Only Setters feature of c#
  • remove all mock setups which returning empty SemanticVersionPreReleaseTag and SemanticVersionBuildMetaData

What do you think? (Maybe in a separate PR)

@arturcic
Copy link
Member

arturcic commented Jan 9, 2023

@HHobeck I can take this one in a separate PR:

removing the .net standard target framework and change it to .net 6.0 and .net 7.0

As for the others, you can work on them if you have a bit of time

@AlexPykavy
Copy link
Contributor Author

@AlexPykavy Could you please take a look on all references of the property PreReleaseTag and BuildMetaData and remove the following null checks?

  • e.g. PreReleaseTag?.HasTag(), PreReleaseTag?.Number, PreReleaseTag == null and PreReleaseTag.ShouldNotBeNull()
  • e.g. BuildMetaData?.Sha, BuildMetaData == null and PreReleaseTag.ShouldNotBeNull()

Done if I understood you correctly :)

@HHobeck
Copy link
Contributor

HHobeck commented Jan 11, 2023

Done if I understood you correctly :)

It looks quite good. :) Could you take a look to the following lines?

image

and

image

@AlexPykavy AlexPykavy force-pushed the refactor-next-version-calculator branch from 85603d4 to 9b19750 Compare January 11, 2023 15:41
@AlexPykavy
Copy link
Contributor Author

@HHobeck, updated. You got me there: I forgot about checking != null.

@HHobeck
Copy link
Contributor

HHobeck commented Jan 12, 2023

Okay thank your very much @AlexPykavy for your work!! :) From my point of view the PR can be merged to main. @arturcic @asbjornu

@HHobeck HHobeck added this to the 6.x milestone Feb 8, 2023
@HHobeck
Copy link
Contributor

HHobeck commented Feb 19, 2023

I'm not sure why the second tests fails. Maybe you can help me? @arturcic, @AlexPykavy

Is it because of the refactoring in the PR? This two tests are new and comming from the main branch:

image

Message: 
Shouldly.ShouldAssertException : variables.FullSemVer
should be
"1.4.0"
but was
"1.4.0-rc"

…g test

Because release branch has 'beta' prefix configured.
To make the method logic more understandable.
Because `this.mainlineVersionCalculator.FindMainlineModeVersion(nextVersion.BaseVersion)` method
calculates BuildMetaData on its own.
Change next version calculator to act according to the next algorithm:
1. Calculate next version candidate based on the base one
2. Look for the latest `PreReleaseTag` matching Major, Minor and Patch
of the next version candidate.
3. Append the `PreReleaseTag`, if found, to the next version candidate.
4. Compare the next version candidate with `Context.CurrentCommitTaggedVersion`
including the `PreReleaseTag` comparison.
5. Increment the `PreReleaseTag` of the next version candidate if versions mismatch.
6. Return the next version candidate.
…lable type

Because they are always set in SemanticVersion's constructor.
They can be null only if explicitly set by the client of the class and
there are plans to forbid this functionality.
@AlexPykavy AlexPykavy force-pushed the refactor-next-version-calculator branch from 22fa5fc to 5767382 Compare February 19, 2023 12:56
@AlexPykavy
Copy link
Contributor Author

Fixed by considering empty preRelaseTag as a edge case and moving out the corresponding if block. I've also rebased my previous PR on top of current main instead of merging because I like prefer rebase in such cases :)

@arturcic arturcic merged commit ce53e2a into GitTools:main Feb 19, 2023
@mergify
Copy link
Contributor

mergify bot commented Feb 19, 2023

Thank you @AlexPykavy for your contribution!

@arturcic arturcic modified the milestones: 6.x, 6.0.0-beta.1 Mar 2, 2023
@arturcic
Copy link
Member

arturcic commented Mar 2, 2023

🎉 This issue has been resolved in version 6.0.0-beta.1 🎉
The release is available on:

Your GitReleaseManager bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants