Limit trim_trailing_whitespace = true to C# files in EditorConfig

[bamarsha]

Since trim_trailing_whitespace = true was applied to all files, my IDE (Rider) was trimming trailing whitespace in all source files that I edited. This is fine for C# files, since StyleCop enforces that there's no trailing whitespace. But the F# files have not been reformatted yet, so my diffs had a lot of unnecessary whitespace changes.

I would have done a pass over all the F# files to remove trailing whitespace, but Visual Studio's implementation of trim_trailing_whitespace is broken:

• editorconfig trim_trailing_whitespace only applies at end of file
• BUG: editorconfig trim_trailing_whitespace only applies at end of file

This means even after all trailing whitespace is removed, there's no way to make sure that it stays gone, so people using editors that correctly implement EditorConfig will have extraneous whitespace in their diffs if trailing whitespace is accidentally added back by people using Visual Studio. So I'm moving trim_trailing_whitespace = true to just the C# section until we have a way to enforce it across IDEs for F#.

[ScottCarda-MS]

😢 This is most unfortunate. I guess we will just have to be vigilant to check for and remove trailing whitespace in F# files in our PRs. It can be tricky though, and I know I sometimes miss those invisible whitespace characters at the ends of lines.

[ScottCarda-MS]

I wonder if the bug is due to having CRLF line endings. I've noticed Visual Studio doesn't seem to respect setting the line endings to LF in situations where it automatically adds line endings.

[cgranade]

😢 This is most unfortunate. I guess we will just have to be vigilant to check for and remove trailing whitespace in F# files in our PRs. It can be tricky though, and I know I sometimes miss those invisible whitespace characters at the ends of lines.

pre-commit can be used in CI mode to check for trailing whitespace in any language. Check out https://github.com/microsoft/QuantumLibraries/blob/master/.pre-commit-config.yaml for an example.

[bamarsha]

@cgranade Do you need to have pre-commit installed locally to get the automatic fixes? In CI mode do you mean it will fail the build if trailing whitespace exists?

[cgranade]

@cgranade Do you need to have pre-commit installed locally to get the automatic fixes? In CI mode do you mean it will fail the build if trailing whitespace exists?

At least the last time I looked, pre-commit requires local installation for automatic fixes. When run in CI mode, it fails instead of fixing. That said, I can easily imagine writing a GitHub Action that runs in CI mode, and if it fails, runs again in fix mode, and pushes a new commit with those fixes. That way, it wouldn't be another expensive CI gate, but could automatically apply fixes even for users that don't have pre-commit.

[cgranade]

Lolz, someone already made that action: https://github.com/marketplace/actions/pre-commit I haven't investigated it yet, and how well it works with public repos or forks, but it may be something interesting to look into.

[bamarsha]

That looks interesting, @cgranade... I'm not sure how I feel about commits being automatically made and pushed, since it seems like it could introduce surprise merge conflicts when you try to sync your PR branch, but it might be a good solution. I'm going to merge this PR for now to avoid the immediate issue of my editor always stripping all whitespace, but we can try to set up a better solution too.

[cgranade]

That looks interesting, @cgranade... I'm not sure how I feel about commits being automatically made and pushed, since it seems like it could introduce surprise merge conflicts when you try to sync your PR branch, but it might be a good solution. I'm going to merge this PR for now to avoid the immediate issue of my editor always stripping all whitespace, but we can try to set up a better solution too.

Makes sense, @SamarSha! My stray thought wasn't intended to be a suggestion of holding off useful improvements...! Totally agreed about surprise merge conflicts as well, I can see that leading to a lot of additional confusion.