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

Cleanup formatting and style and enforce it in CI #1380

Merged
merged 12 commits into from May 17, 2024

Conversation

mus65
Copy link
Contributor

@mus65 mus65 commented Apr 20, 2024

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 dotnet/roslyn#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.

- enabled IDE0055 to enforce formatting on build
- disabled SA1137 and SA1025 because they are already
  covered by IDE0055
- disabled SA1021 because it conflicts with csharp_space_after_cast
This commit has no manual changes, it is the result
of running "dotnet format whitespace" and "dotnet format style"
@mus65
Copy link
Contributor Author

mus65 commented Apr 20, 2024

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
Copy link

sharwell commented Apr 29, 2024

@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:

*.cs text
*.xaml text

*.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):

*.sln eol=crlf
*.csproj eol=crlf
*.shproj eol=crlf
*.appxmanifest eol=crlf

@mus65
Copy link
Contributor Author

mus65 commented Apr 30, 2024

@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
Copy link

sharwell commented May 13, 2024

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
Copy link
Collaborator

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
Copy link
Contributor Author

mus65 commented May 16, 2024

@sharwell thanks, that seems to work.

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

Copy link
Collaborator

@Rob-Hague Rob-Hague left a comment

Choose a reason for hiding this comment

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

Conflicts incoming...

@Rob-Hague Rob-Hague merged commit c0a353a into sshnet:develop May 17, 2024
1 check was pending
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants