Skip to content

Cleanup formatting and style and enforce it in CI - #1380

Merged
Rob-Hague merged 12 commits into
sshnet:developfrom
mus65:dotnetformat
May 17, 2024
Merged

Cleanup formatting and style and enforce it in CI#1380
Rob-Hague merged 12 commits into
sshnet:developfrom
mus65:dotnetformat

Conversation

@mus65

Copy link
Copy Markdown
Contributor

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.

mus65 added 2 commits April 20, 2024 11:19
- 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
Copy Markdown
ContributorAuthor

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

sharwell commented Apr 29, 2024

Copy link
Copy Markdown

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

*.cstext
*.xamltext

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

*.slneol=crlf
*.csprojeol=crlf
*.shprojeol=crlf
*.appxmanifesteol=crlf

@mus65

Copy link
Copy Markdown
ContributorAuthor

@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

sharwell commented May 13, 2024

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
ContributorAuthor

@sharwell thanks, that seems to work.

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

@Rob-HagueRob-Hague left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Conflicts incoming...

This was referenced Aug 12, 2026
Sign up for freeto 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.

3 participants

@mus65@sharwell@Rob-Hague