-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Update schema to remove internal items and add additional .Net core items #1595
Conversation
@davkean as well. Any other properties\items that we should be adding? |
<xs:complexContent> | ||
<xs:extension base="msb:SimpleItemType"> | ||
<xs:sequence> | ||
<xs:element name="Version" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why no description?
There's going to be plenty to add including all our new properties for assemblyinfo and package generation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm worried about taking more changes. It's fairly low-risk but the guidance we got last week was that it's too late for this kind of fit-and-finish.
@@ -3970,81 +4003,6 @@ elementFormDefault="qualified"> | |||
<xs:documentation><!-- _locID_text="AppxValidateStoreManifest" _locComment="" -->Flag indicating whether to validate store manifest.</xs:documentation> | |||
</xs:annotation> | |||
</xs:element> | |||
|
|||
<!-- Following properties are declared here to get rid of schema validation warnings, --> | |||
<!-- but are not intended for overriding, so they do not have documentation annotations. --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the motivation for getting rid of these? Just that they're no longer relevant since there are no more squiggles?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, as I understand it these are used in Task and Targets and shouldn't really go into project files but were added so that they wouldn't squiggle. Now that we modified the xml language service to opt out of schema validation we don't need them and we don't want them to show up in IntelliSense.
<xs:element name="DotNetCliToolReference" substitutionGroup="msb:Item"> | ||
<xs:annotation> | ||
<xs:documentation> | ||
<!-- _locID_text="DotNetCliToolReference" _locComment="" -->A project-specific tool that can be ran from the command line. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that can be run from the command line
<xs:attribute name="Include" type="xs:string"> | ||
<xs:annotation> | ||
<xs:documentation> | ||
<!-- _locID_text="DotNetCliToolReference_Include" _locComment="" -->Package name of the tool. This may differ from its associated reference package name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand your comment but I'm replacing that with the public documentation for this tag:
"The CLI tool that the user wants restored in the context of the project."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't love that but matching existing strings from elsewhere sounds good.
For assemblyinfo\package properties see dotnet/sdk#2 |
… public docs and add documentation for version
<xs:sequence> | ||
<xs:element name="Version" /> | ||
</xs:sequence> | ||
<xs:attribute name="Include" type="xs:string"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need add Include? If so, then we should add it for PackageReference as well - dont' see that in https://github.com/Microsoft/msbuild/pull/1347/files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Short version - No it's not needed but I'll add it for a better experience.
Long version - Include/Exclude/Remove/Condition/Label show up from inheritance (from things in ItemGroups). You only need to provide it if you want to override the documentation for it. That being said the default message for any of them apply to these (i.e. "Semi-colon separated list of files...").
Looking at other entries in the file only Include is overridden which makes sense given that is the most common one but if we want to improve all usage of this we may want to consider overriding the rest as well.
the current most up to date documentation for pack inputs is here : https://github.com/NuGet/Home/wiki/Adding-nuget-pack-as-a-msbuild-target#pack-target-inputs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looking good. @rrelyea can you look over some of the NuGet-y strings as a sanity check?
<xs:attribute name="Include" type="xs:string"> | ||
<xs:annotation> | ||
<xs:documentation> | ||
<!-- _locID_text="DotNetCliToolReference_Include" _locComment="" -->Package name of the tool. This may differ from its associated reference package name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't love that but matching existing strings from elsewhere sounds good.
@@ -1506,12 +1621,21 @@ elementFormDefault="qualified"> | |||
<xs:annotation> | |||
<xs:documentation><!-- _locID_text="StartupObject" _locComment="" -->Type that contains the main entry point</xs:documentation> | |||
</xs:annotation> | |||
</xs:element> | |||
</xs:element> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
<xs:element name="TargetFramework" type="msb:StringPropertyType" substitutionGroup="msb:Property"/> | ||
<xs:element name="TargetFramework" type="msb:StringPropertyType" substitutionGroup="msb:Property"> | ||
<xs:annotation> | ||
<xs:documentation><!-- _locID_text="TargetFramework" _locComment="" -->Framework that this project targets. Must be a Target Framework Moniker (e.g. netcoreapp1.0)</xs:documentation> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make a bigger deal that it must be a single TFM?
</xs:element> | ||
<xs:element name="PackageIconUrl" type="msb:StringPropertyType" substitutionGroup="msb:Property"> | ||
<xs:annotation> | ||
<xs:documentation><!-- _locID_text="PackageIconUrl" _locComment="" -->The URL for a 64x64 image with transparenty background to use as the icon for the NuGet package in UI display</xs:documentation> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
transparenty?
@dotnet-bot test Windows_NT Build for CoreCLR please |
No description provided.