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

Avoid treating warnings as errors inside the IDE #2727

Merged
merged 1 commit into from
Feb 28, 2018

Conversation

sharwell
Copy link
Contributor

@sharwell sharwell commented Feb 28, 2018

This change helps improve developer inner loop efficiency by revealing but not demanding attention for warnings that occur during normal iterative development. Anything missed will still be caught by the CI and command line builds.

💡 Please do not rebase or squash this pull request on merge.

Copy link
Contributor

@jnm2 jnm2 left a comment

Choose a reason for hiding this comment

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

I've sort of wanted to do this, especially after @OmicronPersei wanted to write the XML docs last instead of first.

<PropertyGroup Condition="'$(BuildingInsideVisualStudio)' != 'true'">
<!-- Ideally this is always enabled, but that tends to hurt developer productivity -->
<TreatWarningsAsErrors>true</TreatWarningsAsErrors>
</PropertyGroup>
Copy link
Contributor

@jnm2 jnm2 Feb 28, 2018

Choose a reason for hiding this comment

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

My preference would have been to wait until two properties shared the condition before adding a new property group:

    <Features>strict</Features>
+
+   <!-- Ideally this is always enabled, but that tends to hurt developer productivity -->
+   <TreatWarningsAsErrors Condition="'$(BuildingInsideVisualStudio)' != 'true'">true</TreatWarningsAsErrors>
+
    <SignAssembly>true</SignAssembly>

But not important.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tend to treat this like a functional unit, which can be copied to other projects as needed. I'll move it if you want me to move it; let me know the decision.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's no problem with it, just curiosity!

@mikkelbu
Copy link
Member

AppVeyor timed out executing the tests. I've restarted it.

Copy link
Member

@mikkelbu mikkelbu left a comment

Choose a reason for hiding this comment

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

LGTM. I don't feel strongly for whether it should be a new property group or just a single element. I'll leave the decision for @jnm2.

@jnm2 jnm2 merged commit f593c55 into nunit:master Feb 28, 2018
@jnm2
Copy link
Contributor

jnm2 commented Feb 28, 2018

Thanks Sam!

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

Successfully merging this pull request may close these issues.

3 participants