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

Enable TreatWarningsAsErrors only on CI #3335

Merged
merged 1 commit into from
Feb 9, 2022

Conversation

Evangelink
Copy link
Member

Description

We raised the severity of some rules from message to warning to ensure they are followed in the team but because of the TreatWarningsAsErrors we end up with some "boring" dev cycles where we are forced to cleanup usings etc while we iterate. The idea is to keep warnings locally to not slow us down but still fail in PRs so that we can keep styling consistency.

cc @nohwnd @MarcoRossignoli

We raised the severity of some rules from message to warning to ensure they are followed in the team but because of the `TreatWarningsAsErrors` we end up with some "boring" dev cycles where we are forced to cleanup usings etc while we iterate. The idea is to keep warnings locally to not slow us down but still fail in PRs so that we can keep styling consistency.
@@ -20,7 +20,9 @@
serialization. This is also defined in build script. -->
<AssemblyVersion Condition="'$(AssemblyVersion)' == ''">15.0.0</AssemblyVersion>

<TreatWarningsAsErrors>true</TreatWarningsAsErrors>
<!-- Treat warnings as errors only on CI to prevent painful development process because of unused usings... -->
<TreatWarningsAsErrors Condition="'$(CIBuild)' == 'true'">true</TreatWarningsAsErrors>
Copy link
Member Author

Choose a reason for hiding this comment

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

@nohwnd I remember you asked if we could disable in draft PRs but except from using github api I couldn't find something already in place.

Copy link
Contributor

@MarcoRossignoli MarcoRossignoli left a comment

Choose a reason for hiding this comment

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

LGTM at first glance...let's see how much pain it will cause and in case we'll revert.

@Evangelink
Copy link
Member Author

I hope it will reduce some pain :P

@MarcoRossignoli
Copy link
Contributor

MarcoRossignoli commented Feb 7, 2022

I hope it will reduce some pain :P

eh...it depends we work usually to have "local as CI" here we're trying to impose the opposite, I think it's ok...let's see how many time we found the "surprise" in the CI...in the night 😄

@Evangelink Evangelink merged commit 4e08e47 into microsoft:main Feb 9, 2022
@Evangelink Evangelink deleted the treatwarningaserrors branch February 9, 2022 08:48
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.

2 participants