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

Changes to our Code Style Guidelines #2128

Closed
siegfriedpammer opened this issue Aug 27, 2020 · 4 comments
Closed

Changes to our Code Style Guidelines #2128

siegfriedpammer opened this issue Aug 27, 2020 · 4 comments

Comments

@siegfriedpammer
Copy link
Member

siegfriedpammer commented Aug 27, 2020

The current code formatting of the ILSpy code base has some problems:

  1. Often lines get very long, due to the following:

    • All our transforms use either pattern matching or out var declarations in combination with long &&/|| chains
    • Developers often use very wide monitors, implicitly encouraging use of the horizontal space
    • We have no maximum line length defined in our style
    • Visual Studio does not support automatically breaking up long lines
  2. Manually breaking up conditions into multiple lines comes with putting the opening brace on the next line for readability - as opposed to all the other expressions where we put the opening brace on the same line.

  3. Historically ILSpy has used the SharpDevelop code formatting style. However, point 2 mentioned above causes problems with it and its use in Visual Studio. The IDE will always put the opening brace on the previous line when trying to reformat the code. This makes using the auto formatter a frustrating experience.

In order to partially solve this problem, we decided to use "opening brace on the next line" for all statements.
In order to make indentation less of a problem, we also decided to stop indenting namespace blocks. However, because this is currently not supported by the Roslyn formatter, we will have to make this change at a later point.

We also discussed adding dotnet-format --check to our build pipeline and we will extend our indentation checker script ("tidy.py") to allow enforcing a maximum line length.

The maximum line length will be set at 110 columns. The reasoning behind this is that with 110 columns it is still possible to view two files side-by-side on an HD screen, which comes in handy when reviewing code.

We also tried clang-format and its Visual Studio extension. The results of the line-breaking algorithm were quite pleasing - as it is highly customizable - however, as it does not support all C# features (for example, LINQ clauses and named arguments are not interpreted properly), it is impossible for us to rely on clang-format, as we make use of named arguments to improve readability in case of bool flags.

Tools to be used:

The reformatting is to be done in a single commit as GIT_AUTHOR="dotnet format <dummy_mail@example.com>". We will add that commit to .git-blame-ignore-revs.

References:

@sharwell
Copy link
Contributor

sharwell commented Aug 27, 2020

The maximum line length will be set at 110 columns. The reasoning behind this is that with 110 columns it is still possible to view two files side-by-side on an HD screen, which comes in handy when reviewing code.

💡 Roslyn's line wrapper is hard-coded to 120 columns, so if you want to use the available refactorings then this will need alteration.

@siegfriedpammer
Copy link
Member Author

siegfriedpammer commented Aug 28, 2020

The maximum line length will be set at 110 columns. The reasoning behind this is that with 110 columns it is still possible to view two files side-by-side on an HD screen, which comes in handy when reviewing code.

💡 Roslyn's line wrapper is hard-coded to 120 columns, so if you want to use the available refactorings then this will need alteration.

Thank you for pointing this out. Enforcing formatting and line length during build is the next step. However, we have a large amount of lines that violate this new rule. I fear we will have to manually adjust all these places, so it will take some time until we can actively enforce it. I am hoping that Visual Studio will have a more advanced and customizable line breaking / aligning algorithm built-in before we reach that point. 😉

@sharwell I am not sure, whether this is a bug or a feature:
when using dotnet-format I have one file of test cases, where some tuple expressions combined with delegates are formatted in an unexpected way:

diff --git a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/TupleTests.cs b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/TupleTests.cs
index a5581f48a..4f4c19dac 100644
--- a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/TupleTests.cs
+++ b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/TupleTests.cs
@@ -92,10 +92,12 @@ public struct GenericStruct<T>
 		public (int, int) AccessRest => (1, 2, 3, 4, 5, 6, 7, 8, 9).Rest;
 
 		public (string, object, Action) TargetTyping => (null, 1, delegate {
-		});
+		}
+		);
 
 		public object NotTargetTyping => ((string)null, (object)1, (Action)delegate {
-		});
+		}
+		);
 
 		public void UnnamedTupleOut(out (int, string, Action, dynamic) tuple)
 		{

Looking at our https://github.com/icsharpcode/ILSpy/blob/master/.editorconfig, would you happen to know the setting that causes this? Should I file a bug report with Roslyn?

Thank you very much!

@sharwell
Copy link
Contributor

sharwell commented Aug 28, 2020

some tuple expressions combined with delegates are formatted in an unexpected way

I'm guessing this is a Roslyn bug.

@christophwille
Copy link
Member

christophwille commented Aug 29, 2020

Notes for build server integration - those links might help:

siegfriedpammer added a commit that referenced this issue Aug 29, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants