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

switch to MinVer #1516

Merged
merged 1 commit into from
Dec 2, 2018
Merged

switch to MinVer #1516

merged 1 commit into from
Dec 2, 2018

Conversation

adamralph
Copy link
Contributor

I see you dropped the analyser meta-package, which made this easy. 🙂

@adamralph adamralph mentioned this pull request Dec 2, 2018
46 tasks
Copy link
Member

@thomaslevesque thomaslevesque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @adamralph! LGTM, I just have a few comments/questions

<Copyright>Copyright (c) FakeItEasy contributors. (fakeiteasyfx@gmail.com)</Copyright>
<PackageProjectUrl>https://fakeiteasy.github.io/</PackageProjectUrl>
<PackageIconUrl>https://fakeiteasy.github.io/img/fakeiteasy_logo_256_square_white.png</PackageIconUrl>
<PackageLicenseUrl>https://github.com/FakeItEasy/FakeItEasy/blob/master/License.txt</PackageLicenseUrl>
<PackageReleaseNotes>https://github.com/FakeItEasy/FakeItEasy/releases</PackageReleaseNotes>
<NoPackageAnalysis>true</NoPackageAnalysis>
<MinVerMajorMinor>5.0</MinVerMajorMinor>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't update this, will it still honor the tags? e.g. when we release 5.1.0, do we need to update MinVerMajorMinor, or is it enough to create a 5.1.0 tag ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. I would prefer not to have to update this ever. Truth be told, I'd rather it not exist at all.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this something that's only necessary until we stop using GitFlow? That is, once we're developing on master where there's a recent ancestor with (for example) a 5.0.0 tag, it'll all work out without this setting?
And any tags we explicitly add for pre-release versions would be honoured?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is all detailed in https://github.com/adamralph/minver#can-i-bump-the-major-or-minor-version.

If we don't update this, will it still honor the tags? e.g. when we release 5.1.0, do we need to update MinVerMajorMinor, or is it enough to create a 5.1.0 tag ?

Yes, it's enough to just create the tag.

Indeed. I would prefer not to have to update this ever. Truth be told, I'd rather it not exist at all.

It's not required. All it does is bump interim builds before the first 5.0.x tag to 5.0.0-alpha.0.{height}. If you prefer, I'll remove it.

Is this something that's only necessary until we stop using GitFlow?

It has nothing to do with your branching strategy. E.g. if you switched to GitHub Flow or Release Flow, there will still be a time when you decide "master is now 5.1.0". If you don't have this property, MinVer will keep calculating 5.0.x version numbers until the first 5.1.x tag.

And any tags we explicitly add for pre-release versions would be honoured?

Absolutely. All this property does is to ensure that the version is no lower than the specified MAJOR.MINOR. At one time during the alpha phase, it was named MinVerMinimumMajorMinor which made that more explicit, but I found the name unwieldy so I changed it do just MinVerMajorMinor. If the longer name would help to avoid question, I guess we could change it back, but my hope is that the FAQ I linked above is enough.

@blairconrad
Copy link
Member

I see you dropped the analyser meta-package, which made this easy.

Yeah, I was going to poke you about it today to see if you were still interested. Thanks!

.gitignore Outdated Show resolved Hide resolved
<Copyright>Copyright (c) FakeItEasy contributors. (fakeiteasyfx@gmail.com)</Copyright>
<PackageProjectUrl>https://fakeiteasy.github.io/</PackageProjectUrl>
<PackageIconUrl>https://fakeiteasy.github.io/img/fakeiteasy_logo_256_square_white.png</PackageIconUrl>
<PackageLicenseUrl>https://github.com/FakeItEasy/FakeItEasy/blob/master/License.txt</PackageLicenseUrl>
<PackageReleaseNotes>https://github.com/FakeItEasy/FakeItEasy/releases</PackageReleaseNotes>
<NoPackageAnalysis>true</NoPackageAnalysis>
<MinVerMajorMinor>5.0</MinVerMajorMinor>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this something that's only necessary until we stop using GitFlow? That is, once we're developing on master where there's a recent ancestor with (for example) a 5.0.0 tag, it'll all work out without this setting?
And any tags we explicitly add for pre-release versions would be honoured?

Copy link
Member

@blairconrad blairconrad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the output distracting:

Restore completed in 961.04 ms for D:\Sandbox\FakeItEasy\tests\FakeItEasy.Specs\FakeItEasy.Specs.csproj.                           
MinVer: Using { Commit: ce44d08, Tag: '5.0.0-alpha.1', Version: 5.0.0-alpha.1, Height: 0 }.                                        
5.0.0-alpha.1                                                                                                                      
MinVer: Using { Commit: ce44d08, Tag: '5.0.0-alpha.1', Version: 5.0.0-alpha.1, Height: 0 }.                                        
5.0.0-alpha.1                                                                                                                      
FakeItEasy.Tests.Approval -> D:\Sandbox\FakeItEasy\tests\FakeItEasy.Tests.Approval\bin\Release\net461\FakeItEasy.Tests.Approval.dll
MinVer: Using { Commit: ce44d08, Tag: '5.0.0-alpha.1', Version: 5.0.0-alpha.1, Height: 0 }.                                        
5.0.0-alpha.1                                                                                                                      
MinVer: Using { Commit: ce44d08, Tag: '5.0.0-alpha.1', Version: 5.0.0-alpha.1, Height: 0 }.                                        
5.0.0-alpha.1                                                                                                                      
MinVer: Using { Commit: ce44d08, Tag: '5.0.0-alpha.1', Version: 5.0.0-alpha.1, Height: 0 }.                                        
5.0.0-alpha.1                                                                                                                      
FakeItEasy.Analyzer.CSharp -> D:\Sandbox\FakeItEasy\src\FakeItEasy.Analyzer.CSharp\bin\Release\FakeItEasy.Analyzer.CSharp.dll

Since later commands spew the nuget package version anyhow, I think the 4.1.0-alpha004.153s sprinkled throughout the build are superfluous.

I tried using <MinVerVerbosity>minimal</MinVerVerbosity> and got this, which was better:

FakeItEasy.Tests.Approval -> D:\Sandbox\FakeItEasy\tests\FakeItEasy.Tests.Approval\bin\Release\net461\FakeItEasy.Tests.Approval.dll              
5.0.0-alpha.1.1                                                                                                                                  
5.0.0-alpha.1.1                                                                                                                                  
5.0.0-alpha.1.1                                                                                                                                  
5.0.0-alpha.1.1                                                                                                                                  
5.0.0-alpha.1.1                                                                                                                                  
FakeItEasy.Analyzer.VisualBasic -> D:\Sandbox\FakeItEasy\src\FakeItEasy.Analyzer.VisualBasic\bin\Release\FakeItEasy.Analyzer.VisualBasic.dll

but not ideal. I assumed <MinVerVerbosity>quiet</MinVerVerbosity> would remove the version numbers from the output altogether, but there was no difference. Am I missing something? Should quiet not be really quiet?
Regardless, I prefer it to the default verbosity.

@adamralph
Copy link
Contributor Author

@blairconrad

I tried using <MinVerVerbosity>minimal</MinVerVerbosity> and got this, which was better:

OK, I'll add that property.

I assumed quiet would remove the version numbers from the output altogether, but there was no difference. Am I missing something? Should quiet not be really quiet?

Unfortunately there is no way to get rid of those until we can address adamralph/minver#112. It's a side effect of using the MSBuild Exec task to execute a command line to give us back the version number. Unfortunately there is no way to make the Exec task suppress stdout going to the console.

@adamralph
Copy link
Contributor Author

I've pushed a fixup to address your comments. Let me know if/when I should squash it.

Also, if you prefer not to use MinVerMajorMinor, just let me know.

@blairconrad
Copy link
Member

Thanks for the gitignore and verbosity change, @adamralph.

I'm inclined to not want the MinVerMajorMinor, but it will set our interim build numbers back to 4.1.0-alpha004.*, which isn't the end of the world, but is not ideal.
Unless @thomaslevesque objects, let's keep it for now. As you say, there's no harm, and having you remove it now or us remove it later will give about the same results.

blairconrad
blairconrad previously approved these changes Dec 2, 2018
@thomaslevesque
Copy link
Member

Unless @thomaslevesque objects, let's keep it for now

👍

@adamralph
Copy link
Contributor Author

Squashed and rebased.

Copy link
Member

@blairconrad blairconrad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @adamralph. I am enjoying the versioning already!

@blairconrad blairconrad merged commit c83026a into FakeItEasy:develop Dec 2, 2018
@blairconrad blairconrad added this to the vNext milestone Dec 2, 2018
@adamralph adamralph deleted the minver branch December 2, 2018 18:02
@adamralph
Copy link
Contributor Author

🎉

@thomaslevesque
Copy link
Member

This change has been released in FakeItEasy 5.0.0.
Thanks @adamralph!

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.

None yet

3 participants