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

Make warnaserror work from the property pages. #2368

Merged
merged 2 commits into from
Jun 2, 2017
Merged

Make warnaserror work from the property pages. #2368

merged 2 commits into from
Jun 2, 2017

Conversation

srivatsn
Copy link
Contributor

@srivatsn srivatsn commented Jun 2, 2017

Customer scenario

If a user goes to the build property pages and enters a warning number in the "Treat Warnings as errors" textbox then those warnings weren't being treated as errors.

This is because the prop page was persisting the warnings to a property called TreatSpecificWarningsAsErrors in the project file - however, the compiler only understands a WarningsAsErrors property. Making the prop page persist the warnings as WarningsAsErrors instead.

@natidea @emgarten - note that this means that NuGet needs to read the WarningsAsErrors property as well. I've updated the nomination rule as well so that we send WarningsAsErrors to NuGet.

Bugs this fixes:

https://devdiv.visualstudio.com/DevDiv/_workitems?id=435264&_a=edit&triage=true

Workarounds, if any

Manually edit the project file.

Risk

Low - just changing the property name.

Performance impact

Low - just changing the property name.

Is this a regression from a previous update?

No

How was the bug found?

VS Feedback - https://developercommunity.visualstudio.com/content/problem/55418/cs4014-warning-as-error.html

@dotnet/project-system

srivatsn added 2 commits June 2, 2017 00:17
…reatSpecificWarningsAsErrors property whereas the compiler just understands WarningsAsErrors property. Swtich the rule to persist WarningsAsErrors
@srivatsn
Copy link
Contributor Author

srivatsn commented Jun 2, 2017

Tagging @MattGertz for approval.

@@ -106,7 +106,7 @@
Visible="False"
ReadOnly="True" />

<StringProperty Name="TreatSpecificWarningsAsErrors"
<StringProperty Name="WarningsAsErrors"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to send both properties to NuGet? What if someone specifically enters <TreatSpecificWarningsAsErrors>NU1000</TreatSpecificWarningsAsErrors>? Or do we need to deprecate that because it will not be reflected in the property page?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya TreatSpecificWarningsAsErrors is not a property that has meant anything to anyone in the past. It looks like the DTE property is called that and so we just had a StringProperty with that same name by mistake. The only projects that will have this are ones where someone tried to set WarnAsError from the proppages and it wouldn't have worked. So its dead code in their project files. I don't see any reason to support both.

Copy link
Contributor

@natidea natidea Jun 2, 2017

Choose a reason for hiding this comment

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

OK, we should update the NuGet spec at https://github.com/NuGet/Home/wiki/Improved-NuGet-warnings to use <WarningsAsErrors> in Scenario-1, step 5
/cc @anangaur

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we should. Tagging @emgarten again to make sure that the implementation does that.

@srivatsn
Copy link
Contributor Author

srivatsn commented Jun 2, 2017

Tagging @MattGertz for approval.

@srivatsn srivatsn merged commit bfaefb3 into dotnet:master Jun 2, 2017
@srivatsn srivatsn deleted the warnaserror branch June 2, 2017 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants