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

CSharpier always includes indentation whitespace on non-content lines #1455

Open
rcdailey opened this issue Jan 20, 2025 · 5 comments
Open

Comments

@rcdailey
Copy link

Input:

public class CSharpierFormattingTest
{
    public void Input()
    {
        const int number = 100;

        Console.WriteLine(
            $"""

            ===========================================
            Processing Something {number}
            ===========================================

            """
        );
    }
}

Output:

Screenshot of git diff output to visualize the whitespace characters added:

Image

Patch of the diff:

diff --git src/Recyclarr.Cli/Processors/Sync/SyncProcessor.cs src/Recyclarr.Cli/Processors/Sync/SyncProcessor.cs
index 5241d405..18215c6a 100644
--- src/Recyclarr.Cli/Processors/Sync/SyncProcessor.cs
+++ src/Recyclarr.Cli/Processors/Sync/SyncProcessor.cs
@@ -84,7 +84,7 @@ public class SyncProcessor(
                     ===========================================
                     Processing {config.ServiceType} Server: [{config.InstanceName}]
                     ===========================================
-
+                    
                     """
                 );
 

Expected behavior:

CSharpier should not introduce trailing whitespace inside raw string literals (denoted by triple quotes """) in C#, specifically for "blank lines" within these strings. The expected output should maintain the original formatting without adding any trailing whitespace characters. In this case, the blank line within the raw string literal should remain empty without any added spaces.

The expected diff should show no changes to the blank line within the raw string literal:

                     ===========================================
                     Processing {config.ServiceType} Server: [{config.InstanceName}]
                     ===========================================

                     """
                 );
@belav
Copy link
Owner

belav commented Jan 20, 2025

According to the spec for raw string literals the behavior of CSharpier is correct - see https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/tokens/raw-string

If a whitespace precedes the end delimiter on the same line, the exact number and kind of whitespace characters (e.g. spaces vs. tabs) must exist at the beginning of each content line. Specifically, a space does not match a horizontal tab, and vice versa.

That preceding whitespace is not included in the actual string content and it may be that the compiler isn't strict about enforcing that each content line begin with it. Or maybe it isn't required when the content line is just a new line.

@rcdailey
Copy link
Author

rcdailey commented Jan 20, 2025

A "content line" would be a line with content, i.e. actual text in the string. I think in this case, it is not a content line, and as such, is treated as the usual trailing space at the end of a line that can be removed.

When I run the above code, I get proper behavior in my rendered output (a blank line with nothing on it).

For example this is a "content line":

                     ===========================================

It contains the text ===========================================.

The line below it, is not a "content line" as I understand it. There's nothing there. Just a newline. I don't think the spec you linked meant that we intentionally keep literal newlines indented as well.

@rcdailey
Copy link
Author

rcdailey commented Jan 20, 2025

I actually missed this part:

If a whitespace precedes the end delimiter on the same line

It's talking about the whitespace on the same line as the closing triple-quotes, I believe. It doesn't mean on the line before, it means before on the same line. In other words, the text column where the final/closing triple quote characters start determines how much indentation is subtracted from the start of all lines in the string.

I will admit the wording on the page you linked is a little tricky and nuanced. But I don't think it directly applies to the bug I reported.

@belav belav changed the title CSharpier adds trailing whitespace in raw string literals CSharpier always includes indentation whitespace on non-content lines Jan 20, 2025
@belav
Copy link
Owner

belav commented Jan 20, 2025

I found a better link that clarifies things - https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-11.0/raw-string-literal

They don't consider lines with only whitespace to be content lines. In my mind a blank line was still content because it ends up in the output. If a line is empty or only has whitespace (a non-content line) the initial whitespace will be trimmed based on the end delimiter. Some examples here - https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-11.0/raw-string-literal#example-5---empty-blank-line

CSharpier adding the indentation whitespace to non-content lines keeps the behavior of the code the same, because all of that whitespace ends up trimmed.

So CSharpier could handle it in three ways

  1. Ensure that a non-content line always includes the indentation whitespace (current behavior)
  2. Remove whitespace on non-content lines if there is no extra whitespace after the indentation whitespace
  3. Leave whitespace alone completely on non-content lines (sounds most like what you asked for, although number 2 would probably also work)

One argument for number 1, is that if you want to add content to what used to be a blank line, you don't have to start by adding the indentation whitespace.

Number 3 kind of leaves it up to the developer. From playing around in rider the IDE will auto include the indentation whitespace when you hit enter.

Number 2 - not sure I love it. If the IDE auto indents a new line when I hit enter. Then I hit save to format on save I lose that indentation. Then I'm forced to re-add it myself if I did want content on that line.

I'm leaning towards changing the behavior to number 3 after reading about it more and thinking it through. I do want to scan through some repos to see how common it may be to have no-whitespace vs always indentation whitespace on non-content lines.

@rcdailey
Copy link
Author

I want to provide some additional context regarding my concerns, which aren't centered on C# behavior directly:

  • Trailing whitespace is typically flagged as an "error" by Git, which is why it's highlighted in red when you run git diff.
  • Many tools, such as VS Code and Rider, offer options to automatically trim trailing whitespace on save. This can lead to a conflict with CSharpier, where CSharpier keeps adding the whitespace that these tools remove, causing files to change repeatedly.

Regarding the potential approaches CSharpier could adopt:

I'm in favor of option 2. Given that CSharpier already removes trailing whitespace on code lines, it seems consistent and logical to handle this case similarly. The reasons are twofold: it aligns with the general coding practice to avoid non-functional whitespace, and in this context, the whitespace doesn't contribute to any functionality as per C#.

As for the argument backing option 1, where some see value in retaining indentation whitespace for ease of adding content later, I believe this falls outside the scope of what CSharpier should manage. I prefer CSharpier to focus on the current state of the code. Editing workflows are better handled by IDEs, which typically offer assistance with correct indentation when adding new lines of code.

On the topic of option 3, while it's left to the discretion of the developer, it poses a risk of inconsistency across different development environments, based on IDE configurations. This is precisely the kind of variability CSharpier aims to minimize.

Thus, I disagree with option 3, as managing indentation is an IDE-level concern rather than something that should be controlled by CSharpier.

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

No branches or pull requests

2 participants