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 FormattingAnalyzer #31142

Merged
merged 7 commits into from
Dec 14, 2018
Merged

Conversation

sharwell
Copy link
Member

@sharwell sharwell commented Nov 13, 2018

Recommend reviewing by individual commit.

Reviewers should pay particular attention to all commits that are not the large and automated application of formatting changes.

@sharwell sharwell added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Nov 13, 2018
@sharwell sharwell requested review from a team as code owners November 13, 2018 19:52
// diagnostics have been reported (not necessarily on this thread).
done:
// Don't return until we've seen all of the CompletionParts. This ensures all
// diagnostics have been reported (not necessarily on this thread).
Copy link
Member

Choose a reason for hiding this comment

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

likely formatting bug.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, this appears to be a bug.

@@ -1122,7 +1122,7 @@ static S1 MayWrap(ref int arg)
}
";
CreateCompilationWithMscorlibAndSpan(text).VerifyDiagnostics(
// no diagnostics expected
// no diagnostics expected
Copy link
Member

Choose a reason for hiding this comment

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

this change is minor, but seems also undesirable.

Copy link
Member

Choose a reason for hiding this comment

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

probably not a big deal though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree this appears to be a bug.

@CyrusNajmabadi

This comment has been minimized.

@sharwell sharwell force-pushed the formatting-analyzer branch from ec5d28b to a979978 Compare November 14, 2018 00:14
@CyrusNajmabadi
Copy link
Member

Remove trailing whitespace

Commit looks good! :)

@sharwell sharwell force-pushed the formatting-analyzer branch from a979978 to 33199ac Compare November 14, 2018 14:08
@CyrusNajmabadi
Copy link
Member

Went through all the change. LGTM. Thanks for breaking it out into pieces. It made reviewing much easier!

@sharwell sharwell removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Dec 6, 2018
@jaredpar

This comment has been minimized.

@sharwell

This comment has been minimized.

@sharwell sharwell force-pushed the formatting-analyzer branch from 187686d to b22a9ac Compare December 6, 2018 18:43
@sharwell
Copy link
Member Author

sharwell commented Dec 6, 2018

@jaredpar @tmat The only substantial difference in the last force-push is I rewrote daf181f12ccfcdd62fe94896d963a493eae04481 to account for the switch to arcade.

@@ -413,7 +413,7 @@ function MSBuild() {
$cmdArgs = "$($buildTool.Command) /m /nologo /clp:Summary /v:$verbosity /nr:$nodeReuse"

if ($warnAsError) {
$cmdArgs += " /warnaserror /p:TreatWarningsAsErrors=true"
$cmdArgs += " /p:TreatWarningsAsErrors=true"
Copy link
Member Author

Choose a reason for hiding this comment

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

@tmat I had to make this change or the build would ignore my request for <WarningsNotAsErrors>.

@jaredpar

This comment has been minimized.

Copy link
Member

@jaredpar jaredpar left a comment

Choose a reason for hiding this comment

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

Need to disable the analyzer warnings on only the build correctness leg.

@sharwell sharwell force-pushed the formatting-analyzer branch from b22a9ac to 25a63ee Compare December 10, 2018 20:12
@sharwell

This comment has been minimized.

@jaredpar

This comment has been minimized.

This changes the code style analyzers so that they only execute on the
build correctness leg. This means that even in the face of style errors
our build will be covered via this leg but we will also get test run
output as the other legs won't hit the style errors.
@jaredpar
Copy link
Member

Pretty sure this commit should do the trick

jaredpar@c621d52

@sharwell
Copy link
Member Author

@jinujoseph for approval

@sharwell sharwell merged commit fd96539 into dotnet:master Dec 14, 2018
@sharwell sharwell deleted the formatting-analyzer branch December 14, 2018 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants