sshnet / SSH.NET

SSH.NET is a Secure Shell (SSH) library for .NET, optimized for parallelism.
MIT License
3.87k stars 915 forks source link

Cleanup formatting and style and enforce it in CI #1380

Closed mus65 closed 1 month ago

mus65 commented 1 month ago

I have "format on save" enabled in VS Code and have been frequently annoyed that this often causes changes in code that I didn't actually touch. This is due to a lot of formatting inconsistencies in the codebase where the .editorconfig setting doesn't actually match the code.

Formatting is currently not enforced in CI because IDE0055 (fix formatting) has been disabled by @drieseng because of https://github.com/dotnet/roslyn/issues/63256 . While I understand the decision, imho this one style change is not worth dealing with unnecessary code changes and not being able to enforce formatting in CI.

This PR has two separate commits. The first one makes the necessary changes to the .editorconfig to enable IDE055 and disable some StyleCop analyzers that are either redundant or conflict with other settings. The second commit actually cleans up the code base by running "dotnet format whitespace" and "dotnet format style" and has no further manual changes.

mus65 commented 1 month ago

The Windows CI failures make no sense, I'm pretty sure this is a bug in roslyn. This is somehow caused by the combination of using CRLF and having a preprocessor directive directly after an XML comment, really weird.

Only workaround that I see would be to configure git to always checkout *.cs files with LF, even on Windows.

I'm looking into reporting this to roslyn.

sharwell commented 1 month ago

@mus65 Can you try adding the following to your appveyor configuration? I believe that without it, appveyor is checking out your files with LF line endings instead of CRLF line endings. Most Windows users will already have configured this option locally so it would not be a problem outside of CI.

https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/17b613ddbc2f0f8313727953048cea30c36b7482/appveyor.yml#L3-L4

I would also suggest some slight alterations to these lines:

https://github.com/sshnet/SSH.NET/blob/59840ecdbaad0a0df8a56de71dfb05d3d7896942/.gitattributes#L3-L4

*.cs text=auto
*.xaml text=auto

Note that in general, I would not recommend specifying a line ending style in either .gitattributes or .editorconfig. The exception to this is files which by definition require a specific line ending form that applies regardless of operating system, such as .sh files. This means all of the following can likely be removed (at which point the initial * text=auto line will apply to them):

https://github.com/sshnet/SSH.NET/blob/59840ecdbaad0a0df8a56de71dfb05d3d7896942/.gitattributes#L5-L8

mus65 commented 1 month ago

@sharwell setting autocrlf indeed fixed Windows CI, but now Linux CI is broken with the same errors instead, see https://ci.appveyor.com/project/drieseng/ssh-net/builds/49723086 .

I'm even more confused now. I still can't reproduce this locally on windows, even when disabling autocrlf in the global git config and re-cloning the repo.

I would also suggest some slight alterations to these lines: .cs text=auto .xaml text=auto

Would this actually change anything here? From my understanding this would just cause git to autodect whether .cs is text instead of always treating it as text (since `.cs text` is currently set). Conversion of line endings happens either way.

sharwell commented 1 month ago

It looks like you are setting core.autocrlf on both the Windows and Linux tests. If you want to set the value on Linux tests, it should be set to input:

Windows

git config --global core.autocrlf true

Linux

git config --global core.autocrlf input
Rob-Hague commented 1 month ago

Feel free to change the cast rule depending on your preference (mine would be to change it)

diff --git a/.editorconfig b/.editorconfig
index d3d940a8..abe8bd6f 100644
--- a/.editorconfig
+++ b/.editorconfig
@@ -925,7 +925,7 @@ csharp_indent_block_contents = true

 # Spacing options

-csharp_space_after_cast = true
+csharp_space_after_cast = false
 csharp_space_after_keywords_in_control_flow_statements = true
 csharp_space_between_parentheses = false
 csharp_space_before_colon_in_inheritance_clause = true
mus65 commented 1 month ago

@sharwell thanks, that seems to work.

@Rob-Hague I prefer this is as well, pushed.