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.

Sign up for GitHub

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

Jump to bottom

Typo fix #1687

Merged
merged 4 commits into from
Feb 11, 2017
Merged

Typo fix #1687

merged 4 commits into from
Feb 11, 2017

Conversation

cdmihai
Copy link
Contributor

@cdmihai cdmihai commented Feb 10, 2017

No description provided.

@RaulPerez1
Copy link
Contributor 

    <xs:documentation><!-- _locID_text="VersionSufix" _locComment="" -->Suffix that can indicate additional information at the end of the verion (i.e. beta)</xs:documentation>

Typo here as well

Refers to: src/MSBuild/Microsoft.Build.CommonTypes.xsd:1804 in c2aac57. [](commit_id = c2aac57, deletion_comment = False)

RaulPerez1
RaulPerez1 reviewed Feb 10, 2017
View reviewed changes
src/MSBuild/Microsoft.Build.CommonTypes.xsd Outdated
@@ -1796,7 +1796,7 @@ elementFormDefault="qualified">
</xs:element>
<xs:element name="VersionPrefix" type="msb:StringPropertyType" substitutionGroup="msb:Property" >
<xs:annotation>
<xs:documentation><!-- _locID_text="VersionPrefix" _locComment="" -->Prefix that can indicate additional information at the end of the verion (i.e. beta)</xs:documentation>
<xs:documentation><!-- _locID_text="VersionPrefix" _locComment="" -->Prefix that can indicate additional information at the start of the version (i.e. beta)</xs:documentation>
Copy link
Contributor

Choose a reason for hiding this comment

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

i.e. [](start = 154, length = 4)

These are supposed to be e.g. Same goes for the one below

@RaulPerez1
Copy link
Contributor

:shipit:

rainersigwald
rainersigwald requested changes Feb 10, 2017
View reviewed changes
src/MSBuild/Microsoft.Build.CommonTypes.xsd Outdated
@@ -1796,7 +1796,7 @@ elementFormDefault="qualified">
</xs:element>
<xs:element name="VersionPrefix" type="msb:StringPropertyType" substitutionGroup="msb:Property" >
<xs:annotation>
<xs:documentation><!-- _locID_text="VersionPrefix" _locComment="" -->Prefix that can indicate additional information at the end of the verion (i.e. beta)</xs:documentation>
<xs:documentation><!-- _locID_text="VersionPrefix" _locComment="" -->Prefix that can indicate additional information at the start of the version (i.e. beta)</xs:documentation>
Copy link
Member

Choose a reason for hiding this comment

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

Looking at how this is used, I'm not sure this is a good description.

    <VersionPrefix Condition=" '$(VersionPrefix)' == '' ">1.0.0</VersionPrefix>
    <Version Condition=" '$(VersionSuffix)' != '' ">$(VersionPrefix)-$(VersionSuffix)</Version>
    <Version Condition=" '$(Version)' == '' ">$(VersionPrefix)</Version>

So maybe this should be Initial part of the version number, for example 1.0.0 ?

@nguerrera @eerhardt y'all seem to have been the ones to touch this in the SDK, do you have opinions?

Copy link
Member

Choose a reason for hiding this comment

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

Agreed this is not a good description for this property.

This is the best documentation I can find that explains these properties: https://github.com/dotnet/cli/issues/5573#issuecomment-277377729

Copy link
Contributor Author

@cdmihai cdmihai Feb 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

How about:

  • VersionPrefix: When Version is not specified, VersionPrefix represents the first fragment of the version number (e.g. 1.0.0). The syntax is <VersionPrefix>[-<VersionSuffix>]
  • VersionSuffix: Optional. When Version is not specified, VersionSuffix represents the second fragment of the version number (e.g. beta). The syntax is <VersionPrefix>[-<VersionSuffix>]

Copy link
Member

Choose a reason for hiding this comment

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

Seems reasonable to me . . .

Copy link
Member

Choose a reason for hiding this comment

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

👍

@cdmihai
Copy link
Contributor Author

cdmihai commented Feb 10, 2017

@RaulPerez1 we changed the description for VersionSuffix and VersionPrefix. Still look good to you?

@RaulPerez1
Copy link
Contributor

Looks good to me.

@cdmihai
Copy link
Contributor Author

cdmihai commented Feb 10, 2017

@RaulPerez1
Oops, I had forgotten to commit my changes when I pinged you. What about now? :)

rainersigwald
rainersigwald approved these changes Feb 10, 2017
View reviewed changes
src/MSBuild/Microsoft.Build.CommonTypes.xsd Outdated
@@ -1796,12 +1796,12 @@ elementFormDefault="qualified">
</xs:element>
<xs:element name="VersionPrefix" type="msb:StringPropertyType" substitutionGroup="msb:Property" >
<xs:annotation>
<xs:documentation><!-- _locID_text="VersionPrefix" _locComment="" -->Prefix that can indicate additional information at the end of the verion (i.e. beta)</xs:documentation>
<xs:documentation><!-- _locID_text="VersionPrefix" _locComment="" --> When Version is not specified, VersionPrefix represents the first fragment of the version string (e.g. 1.0.0). The syntax is VersionPrefix[-VersionSuffix].</xs:documentation>
Copy link
Member

Choose a reason for hiding this comment

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

nit: leading space.

@RaulPerez1
Copy link
Contributor

Thumbs up!

@cdmihai cdmihai merged commit 487db4a into xplat Feb 11, 2017
@cdmihai cdmihai deleted the cdmihai/typoFixes branch February 11, 2017 02:04
AndyGerlicher pushed a commit to AndyGerlicher/msbuild that referenced this pull request Feb 23, 2017
@cdmihai @AndyGerlicher
Better description for VersionSuffix and VersionPrefix (dotnet#1687)
4eba32f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eerhardt eerhardt eerhardt left review comments

@rainersigwald rainersigwald rainersigwald approved these changes

@RaulPerez1 RaulPerez1 RaulPerez1 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@cdmihai @RaulPerez1 @rainersigwald @eerhardt @dnfclas