Run dotnet-format

[ryzngard]

This runs dotnet-format -w Roslyn.sln and adds the commit to the new .git-blame-ignore-revs file

I put this up to both drive my desire for us to get "clean" by dotnet-format standards, and to improve format if needed. I fully expect discussion on this and should require more general sign off than needed for changes.

The benefits of this change:

• We standardize whitespace to our tooling.
• Uses can easily use dotnet-format on their own branches/commits now without worrying about including other changes
• Gets us closer to allowing dotnet-format apply in CI?

Negatives:

• Blame will be messed up if --ignore-revs is not used
• Large change for formatting only

To set up git to use the --ignore-revs by default run git config blame.ignoreRevsFile .git-blame-ignore-revs

Open questions/issues:

• Does Visual Studio support --ignore-revs with git?
• VS Code GitLens has ignore-revs support
• GitHub UI blame does not support --ignore-revs
• Other tooling considerations?

[ryzngard]

I'm marking this as Blocked until explicit signoff is more broad. This will not go in until we have general agreement.

We can also do this in smaller scale for specific folders at a time

[RikkiGibson]

.git-blame-ignore-revs file

Oh my god I'm so glad this exists. That means we might actually be able to adopt "file scoped namespaces" once that ships.

[JoeRobich]

This would allow us to dogfood the proposed GitHub action (or AzDO task) which validates a passing dotnet-format

[sharwell]

📝 Missing trailing newline

[sharwell]

❗ It appears that dotnet-format is applying style changes that are not part of the repository validation (e.g. using directives changes). Marking as blocked pending cross-team agreement.

[sharwell]

Uses can easily use dotnet-format on their own branches/commits now without worrying about including other changes

This was already the case for formatting. Since CI enforces formatting for every pull request, there is no risk that a branch has to redo formatting for code outside the branch.

[ryzngard]

Uses can easily use dotnet-format on their own branches/commits now without worrying about including other changes

This was already the case for formatting. Since CI enforces formatting for every pull request, there is no risk that a branch has to redo formatting for code outside the branch.

What tool can I run locally to fix this formatting? Why is it not dotnet-format, which we design and ship specifically for this?

[JoeRobich]

❗ It appears that dotnet-format is applying style changes that are not part of the repository validation (e.g. using directives changes).

Opened dotnet/format#1175 for this issue

[AlekseyTs]

I am not sure is we want any automatic formatting done under Compilers. Frankly, this change comes as a surprise to me.

[ryzngard]

@AlekseyTs that is fine if that's the stance of the compiler team. This can be modified to exclude the Compilers folder

[jaredpar]

The Compiler team stance is, primarily, we'd like to find out about proposed formatting changes in some other way than a PR: email, chat, etc ... This has been our repeated ask over the years.

Putting style changes as a PR has a strong tendency to make the discussion more contentious. Hence we'd like to first chat about what changes are being made, why they're being made and evaluate if, and when, we would want to take them.

For example I'd personally push back strongly on this, or any other large style change, right now because we're in the middle of many large feature merges. A style change right now is risking our feature shipping schedule.

[ryzngard]

@jaredpar I understand. I had hoped this to serve as a point of discussion for "what is changing based on our current rules" . The goal of dotnet-format is to enforce existing rules, and that was the goal of this pr.

I did not mean to be disruptive, or at least to minimize disruption. I understand that large formatting changes like this are hard. I had intended this to be a discussion of what the actual changes are so we have hard data on what is changing by running tools that we ship. There is no intent to change styling guidelines here.

Timing is also important, and I'm happy to discuss when this change would be better suited. Again, I meant no disrespect or devious action by opening this PR. My intent was to open a discussion with actual changes being made rather than open an issue around hypothetical. Out of this PR alone we are making improvements to dotnet-format to better suit the design. I count that as a success regardless of the merge status of this PR.

I started a more private chat when I opened this PR. If we need to talk offline I'm always open to direct messages or responding to that conversation. In no way am I trying to undermine or deviate from decisions made by the team. I'm trying to apply our current tooling to our repo in a way that makes sense. Dotnet-format is a tool that is listed as a dependency in this repo but not usable in it's current state. I want it to be usable, and for developers to benefit across the board.

I appreciate the feedback you've provided and will try to be more adamant about discussion prior to opening a PR. This is a miss on my part due to thinking that a PR was a good place for discussion.

[ryzngard]

With new change to dotnet-format to treat usings ordering as codestyle, whitespace only changes updated. Much smaller :)

[RikkiGibson]

I couldn't tell what's actually getting changed. Are BOMs getting normalized or something like that?

[ryzngard]

Yea, it may be a little easier to see with git diff

diff --git a/src/Compilers/CSharp/Portable/Binder/MethodArgumentInfo.cs b/src/Compilers/CSharp/Portable/Binder/MethodArgumentInfo.cs
index 5e742d84d72..b8a2f76c02e 100644
--- a/src/Compilers/CSharp/Portable/Binder/MethodArgumentInfo.cs
+++ b/src/Compilers/CSharp/Portable/Binder/MethodArgumentInfo.cs
@@ -1,4 +1,4 @@
-// Licensed to the .NET Foundation under one or more agreements.
+<EF><BB><BF>// Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.

[JoeRobich]

Our .editorconfig specifies utf8-bom as the charset. These changes add the necessary BOM.

[jmarolf]

♥

[jcouv]

LGTM Thanks (iteration 6)

Talked offline to dismiss