Skip to content

Remove trailing whitespace and enforce StyleCop rule SA1028#625

Merged
jamill merged 2 commits into
microsoft:masterfrom
jamill:stylecop_rules
Dec 27, 2018
Merged

Remove trailing whitespace and enforce StyleCop rule SA1028#625
jamill merged 2 commits into
microsoft:masterfrom
jamill:stylecop_rules

Conversation

@jamill
Copy link
Copy Markdown
Member

@jamill jamill commented Dec 14, 2018

@kewillford had suggested that we open issues to start driving down the number of rules that we have disable in our StyleCop ruleset - and this is a step in that direction. I wanted to see how people felt about this rule, fixing up violations in our code base, and seeing if this will cause disruption to work that is currently in flight. My hope is that the diff itself will be easy to look through, as it only contains whitespace differences for all but 1 file (GVFS.ruleset, where I remove the override for this rule).

Clean up existing instances of trailing whitespace in our codebase and
stop suppressing StyleCop rule SA1028 which enforces no trailing
whitespace:

https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1028.md

These changes were generated by applying the StyleCop automatic fixup for SA1028.

Copy link
Copy Markdown
Contributor

@derrickstolee derrickstolee left a comment

Choose a reason for hiding this comment

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

I think this is a good thing to do! It will be easy for users to fix the problems now that we have the Roslyn-based stylecop.

The only issue I see is the (temporary) struggle of possible merge conflicts. This can be fixed by local rebase or merge that should be easy to do.

@jeschu1
Copy link
Copy Markdown
Member

jeschu1 commented Dec 14, 2018

So my concern is that for history a ton of stuff will have your name on it. Maybe it's just a concern for @jamill really :-).

I thought GitHub had a blame again feature but I'm not seeing it.

@jamill
Copy link
Copy Markdown
Member Author

jamill commented Dec 14, 2018

So my concern is that for history a ton of stuff will have your name on it

@jeschu1 - yes, this is a concern. This is (somewhat) mitigated by:

  • you can use git blame -w to ignore whitespace changes (this is not obvious, or might not help until you actually run into a problem).

  • On the github blame UI, you can view the previous blame.

I would be interested if people think this is worth the trade-off.

@jamill
Copy link
Copy Markdown
Member Author

jamill commented Dec 14, 2018

The other trade off is we deal with non-functional whitespace changes as we encounter them in individual PRs. not as drastic of a single change, but the impact is spread out over time.

@wilbaker
Copy link
Copy Markdown
Member

wilbaker commented Dec 14, 2018

UPDATE @jamill I misread your comment the first time, updating mine 😄

The other trade off is we deal with non-functional whitespace changes as we encounter them in individual PRs. not as drastic of a single change, but the impact is spread out over time.

I agree that it's better to do this all at once.

I would be interested if people think this is worth the trade-off.

@jamill, are there settings and\or extensions you can recommend for VS, VS for Mac, and VS Code that will strip trailing whitespace automatically? If that can be configured then I'm in favor of making the change in this PR.

@jamill
Copy link
Copy Markdown
Member Author

jamill commented Dec 14, 2018

are there settings and\or extensions you can recommend for VS, VS for Mac, and VS Code that will strip trailing whitespace automatically?

Great question -

The StyleCop will flag trailing whitespace, and offer an option to fix it "everywhere", but it is a manual action.

There is an .editorconfig configuration we could use, that looks like it is supported in different editors, but I have not tried it everywhere. In playing around in Visual Studio (for windows), it looks like it will trim trailing whitespace when pressing "enter" on a line, but not necessarily trim it on save. I will play with this some more and see what other options are out there.

@nickgra
Copy link
Copy Markdown
Member

nickgra commented Dec 14, 2018

I use .editorconfig in other projects in VSCode and it works great. If we make this change (and I think we should), we should also set up .editorconfig. I've heard it also works in VS for Windows and Mac (docs for Mac here: https://docs.microsoft.com/en-us/visualstudio/mac/editorconfig).

Notably, Xcode doesn't support .editorconfig so we'll need to keep in mind that our rules won't be applied to C++ code in ProjFS.Mac.

Copy link
Copy Markdown
Member

@kewillford kewillford left a comment

Choose a reason for hiding this comment

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

Since my name is on most blames because I was the last to push all the change to GitHub I am all for this change so some of the blame goes to @jamill. :)

@wilbaker
Copy link
Copy Markdown
Member

Notably, Xcode doesn't support .editorconfig so we'll need to keep in mind that our rules won't be applied to C++ code in ProjFS.Mac.

@jamill @nickgra I found this setting in Xcode:

image

In XCode -> Preferences -> Text Editing.

However, I'm guessing StyleCop won't apply to these files anyway.

There is an .editorconfig configuration we could use, that looks like it is supported in different editors, but I have not tried it everywhere. In playing around in Visual Studio (for windows), it looks like it will trim trailing whitespace when pressing "enter" on a line, but not necessarily trim it on save. I will play with this some more and see what other options are out there

The .editorconfig sounds like a good option, can that file be checked into the repo?

@nickgra
Copy link
Copy Markdown
Member

nickgra commented Dec 17, 2018

@wilbaker Yep, in Xcode that's the feature we want turned on, but it won't light up automatically when someone is working in our repo with VSCode or VS2017/2019 like an .editorconfig would. I'll make sure to set that on my machine, but we can't enforce it like Stylecop does on our C# code.

Yes, the .editorconfig should be checked into the repo in the root. Here's an example of one in the VSCode repo: https://github.com/Microsoft/vscode/blob/master/.editorconfig

Since Jameson is out for the rest of the year, would anyone object to me adopting his commits, rebasing them, adding a .editorconfig file and creating a new PR to get this in? I run into this a fair bit and it frustrates me every time.

@wilbaker
Copy link
Copy Markdown
Member

Since Jameson is out for the rest of the year, would anyone object to me adopting his commits, rebasing them, adding a .editorconfig file and creating a new PR to get this in? I run into this a fair bit and it frustrates me every time.

That's good with me, + @jrbriggs for his thoughts.

@jrbriggs
Copy link
Copy Markdown
Member

That's good with me, + @jrbriggs for his thoughts.

@nickgra that'd be awesome, thank you!

Clean up existing instances of trailing whitespace in our codebase and
stop suppressing StyleCop rule SA1028 which enforces no trailing
whitespace:

    https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1028.md
EditorConfig is meant to help define and maintain consistent coding style
across different editors and IDEs. A description for EditorConfig can be found
at:

  https://editorconfig.org

Visual Studio comes with native support for EditorConfig.

  https://docs.microsoft.com/en-us/visualstudio/ide/create-portable-custom-editor-options?view=vs-2017

VS Code doesn't currently support editorconfig out of the box, but it can be
enabled with a plugin. EditorConfig.org lists the following plugin:

  https://marketplace.visualstudio.com/items?itemName=EditorConfig.EditorConfig
@jamill
Copy link
Copy Markdown
Member Author

jamill commented Dec 27, 2018

I rebased these changes onto latest and added an EditorConfig file.

Visual Studio has native support for EditorConfig (documentation)

VS Code does not have native support for EditorConfig, but there is a plug-in that can be installed: EditorConfig for VS Code.

A list of other editors that support EditorConfig can be found at EditorConfig.

One other observation: While Visual Studio for Windows includes an automatic code fix option for the Roslyn Analyzer, I did not see this option for VS for Mac - which was a bit surprising.

/cc @nickgra

@jamill jamill merged commit 25d21da into microsoft:master Dec 27, 2018
@jrbriggs jrbriggs added this to the S147 milestone Feb 7, 2019
@jrbriggs jrbriggs added the affects: engineering Keeping the engineering system healthy label Feb 8, 2019
@jamill jamill deleted the stylecop_rules branch April 3, 2019 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

affects: engineering Keeping the engineering system healthy

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants