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

VersionPrefix and VersionSuffix are misnamed #1952

Closed
adamralph opened this issue Apr 7, 2017 · 10 comments
Closed

VersionPrefix and VersionSuffix are misnamed #1952

adamralph opened this issue Apr 7, 2017 · 10 comments
Labels

Comments

@adamralph
Copy link

adamralph commented Apr 7, 2017

Prefix:

a word, letter, or number placed before another.

The value of this property never prefixes the version. That would imply that the version is another string of characters, and the value of this property precedes it. This never happens.

Until I read the documentation, I was surprised to see values of VersionPrefix of the form "1.0.0", since "1.0.0" is not a prefix to the version. It is the version.

The same argument could be applied to VersionSuffix, depending on the interpretation of "version". If "1.0.0" is the "version" in "1.0.0-beta1" then it's true to say that the "version suffix" is "beta1", but if "1.0.0-beta1" is the "version", then VersionSuffix is similarly misnamed.


Update:

Based on the comments below, it seems that the general opinion is that "1.0.0-beta1" is the "version", implying that VersionSuffix is also misnamed. I'm re-titling the issue from "VersionPrefix is misnamed" to "VersionPrefix and VersionSuffix are misnamed".

@tpluscode
Copy link

tpluscode commented Apr 7, 2017

The naming used here should follow the semver, right?

According to that spec, both 1.2.3 and 2.4.5-beta.2 are versions. The latter being a pre-release version in which what you called VersionSuffix is referred to as dot separated identifiers.

GitVersion uses the term pre-release tag although I'm not sure whether they coined that term. @asbjornu?

@asbjornu
Copy link
Member

asbjornu commented Apr 7, 2017

I think @JakeGinnivan might know better than me where pre-release-tag came from, but as discussed in GitTools/GitVersion#1054, we might change tag to label or identifier (as seems to be the word used by semver) to not confuse it with a git tag.

@dasMulli
Copy link
Contributor

dasMulli commented Apr 7, 2017

fyi the logic comes from https://github.com/dotnet/sdk/blob/master/src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.DefaultAssemblyInfo.targets#L18-L22

It's hard since you need to be able to construct the Version from two parts. Changing the version based on the presence of a VersionSuffix isn't a good option here because it may be a global property and can't be overwritten.
So i guess the feature request would be to create an alias for VersionPrefix (e.g. VersionNumber - but i'm horrible at naming things)

@adamralph
Copy link
Author

adamralph commented Apr 8, 2017

Based on the comments above, it seems that the general opinion is that "1.0.0-beta1" is the "version", implying that VersionSuffix is also misnamed. I'm re-titling the issue from "VersionPrefix is misnamed" to "VersionPrefix and VersionSuffix are misnamed".

@adamralph adamralph changed the title VersionPrefix is misnamed VersionPrefix and VersionSuffix are misnamed Apr 8, 2017
@adamralph
Copy link
Author

adamralph commented Apr 8, 2017

Unfortunately SemVer doesn't define terms to describe the "1.0.0" and "beta1" parts of the version "1.0.0-beta1" so I guess we'll have to define them (or look for precedents elsewhere).

But, before going too far down that path, perhaps we should challenge

you need to be able to construct the Version from two parts

Why?

@dasMulli
Copy link
Contributor

dasMulli commented Apr 8, 2017

you need to be able to construct the Version from two parts

My reasoning is mostly based on the ability and need to pass version-related properties as arguments to msbuild invocations, e.g. from CI.
However, the importance of the different usage scenarios need to be weighted against each other.. Let me elaborate:

You can currently pass /p:VersionSuffix=beta1 and it will generate a 1.0.0-beta1 version (the dotnet cli even has convenience parameters for that). if you set /p:Version=1.2.3;VersionSuffix=beta1, you end up with 1.2.3 - which is expected and you get the same behaviour if you specified these two properties inside a project file or from command line. While not perfectly named, it is at least consistent.

However, take the following project file for example:

<Project>
  <PropertyGroup>
    <Version Condition="'$(Version)' == ''">1.0.0</Version>
    <Version Condition="'$(PreReleaseLabel)' != ''">$(Version)-$(PreReleaseLabel)</Version>
  </PropertyGroup>
  <Target Name="Build">
    <Message Importance="high" Text="Version: $(Version)"/>
  </Target>
</Project>
  1. Invoke it with dotnet msbuild /p:PreReleaseLabel=beta1 and it prints 1.0.0-beta1.
  2. Invoke it using dotnet msbuild /p:Version=1.2.3;PreReleaseLabel=beta1 and you get 1.2.3, because Version is now a global property and can't be overwritten.
  3. Put a <Version>1.2.3</Version> at the top of the property group (pretend everything else is coming from the SDK) and run dotnet msbuild /p:PreReleaseLabel=beta1. The result will be 1.2.3-beta1

My problem with this is that 2. and 3. look similar - both properties set to a value - but behave differently because of the way global msbuild properties work, which is confusing for beginners trying to set up their project / CI.

There are a few workarounds for that but they would require a different property (ResolvedVersion) or each target that needs a version had to look at all available properties. (Not to mention this would be a breaking tooling change).

Summed up, I think it's best to leave Version being composed from two properties, but maybe figure out better names (falling back to the current ones).

@adamralph
Copy link
Author

if you set /p:Version=1.2.3;VersionSuffix=beta1, you end up with 1.2.3...

Therein lies the problem. If I pass Foo, and FooSuffix, I expected the value of Foo to be suffixed by the value of FooSuffix. The current behaviour is surprising and does not follow the semantics conveyed by the property names.

  • which is expected...

It is only expected after you have experienced the surprising behaviour or read the documentation, and you have retained and recalled that knowledge before the occasion. The names (semantics) cannot be relied on to remind the user of the expected behaviour.

I still haven't seen a defense of the assertion that we need the version to be composed of two properties. What does this offer over a single property named Version?

@asbjornu
Copy link
Member

@adamralph:

Unfortunately SemVer doesn't define terms to describe the "1.0.0" and "beta1" parts of the version "1.0.0-beta1" so I guess we'll have to define them (or look for precedents elsewhere).

Yes, SemVer defines them as such:

Additional labels for pre-release and build metadata are available as extensions to the MAJOR.MINOR.PATCH format.

And:

A pre-release version MAY be denoted by appending a hyphen and a series of dot separated identifiers immediately following the patch version.

So SemVer calls these pre-release strings that are suffixed to the version for labels or identifiers. I would call the entire string for SemanticVersion, the version number for VersionNumber and the pre-release suffix for PreReleaseLabel.

@adamralph
Copy link
Author

@asbjornu defining SemanticVersion as {VersionNumber}-{PreReleaseLabel} is not a bad idea. For completeness, I would extend it to {VersionNumber}-{PreReleaseLabel}+{BuildLabel}.

However, I still challenge the need for multiple properties. I haven't seen a strong argument for having multiple properties over a single Version property.

@rainersigwald
Copy link
Member

This issue was moved to dotnet/sdk#1131

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

No branches or pull requests

6 participants