dotnet / format

Home for the dotnet-format command
MIT License
1.94k stars 172 forks source link

Empty lines not removed? #1074

Closed wondering639 closed 3 years ago

wondering639 commented 3 years ago

Just installed dotnet-format and tested with the following code (empty lines intended). Used command was dotnet format Test.sln --fix-whitespace --fix-style. I expected the empty lines to be removed, but they remained. How to apply a more strict formatting that removes unnecessary empty lines?

Tested on this code, which is input and output at the sae time (meaning output remains unchanged)

using System;

namespace Test
{
    using System;
    using System.Collections.Generic;
    using System.Threading;
    using System.Threading.Tasks;

    class Program
    {
        static void Main()
        {
            DemoAsync().Wait();
        }

        static async Task DemoAsync()

        {

            var d = new Dictionary<int, int>();

            for (int i = 0; i < 10000; i++)

            {

                int id = Thread.CurrentThread.ManagedThreadId;

                int count;

                d[id] = d.TryGetValue(id, out count) ? count + 1 : 1;

                await Task.Yield();

            }

            foreach (var pair in d) Console.WriteLine(pair);

        }

    }
}
jmarolf commented 3 years ago

@sharwell is there a fixall in Stylecop analyzers for SA1507?

wondering639 commented 3 years ago

@sharwell is there a fixall in Stylecop analyzers for SA1507?

I just read SA15007 and it's about multiple empty lines following each other directly. In my case it's only one blank line followed by code (well, there's both in my code; expectation would be too remove most of them like e.g. Prettifier in the JavaScript world does it).

sharwell commented 3 years ago

@jmarolf SA1507 does have a code fix (as listed in the status table), but it's not the only rule that applies here. The blank lines before and after most { and } chararacters would be removed by SA1012 and SA1013, respectively.

With that said, none of these rules would apply unless StyleCop Analyzers is installed. 16.10 is the first release where Roslyn's built-in analyzers consider blank line removal, and only in a small set of cases.

wondering639 commented 3 years ago

@sharwell Being new to these things, do I understand correctly that I could add https://github.com/DotNetAnalyzers/StyleCopAnalyzers to my project and dotnet-format would than have more rules/functionality?

JoeRobich commented 3 years ago

do I understand correctly that I could add https://github.com/DotNetAnalyzers/StyleCopAnalyzers to my project and dotnet-format would than have more rules/functionality?

@wondering639 That is right. You can add a package reference to the StyleCopAnalyzers in your projects and fix issues by using the --fix-analyzers option. In fact, we test the StyleCop empty line fixers in our unit tests.

https://github.com/dotnet/format/blob/8b7b3ab3f009de32de53fc8c88baa64684a7adfd/tests/Analyzers/ThirdPartyAnalyzerFormatterTests.cs#L69-L120

dzmitry-lahoda commented 3 years ago

similar request https://github.com/dotnet/format/issues/67

JoeRobich commented 3 years ago

Closing this issue as StyleCopAnalyzers can be used to remove blank lines.

anthony-steele-cko commented 3 years ago

Not working for me. What I did was

Result: SomeClass.cs still has the blank lines afterwards. All I get is Warnings were encountered while loading the workspace.

JoeRobich commented 3 years ago

@anthony-steele-cko You will need to configure the analyzers to report a sufficient severity. You will need to either add or update your editorconfig to include the following configuration. If you do not want them to be errors in your IDE, you can configure them as warnings and invoke dotnet-format with --fix-analyzers warn instead.

.editorconfig

root = true

[*.cs]

# Two or more consecutive blank lines: Remove down to one blank line. SA1507 
dotnet_diagnostic.SA1507.severity = error

# Blank line immediately before or after a { line: remove it. SA1505, SA1509 
dotnet_diagnostic.SA1505.severity = error
dotnet_diagnostic.SA1509.severity = error

# Blank line immediately before a } line: remove it. SA1508 
dotnet_diagnostic.SA1508.severity = error
anthony-steele-cko commented 3 years ago

I confirm that this works for me. This time I have a Directory.Build.Props file containing:

<Project>
  <ItemGroup>
    <PackageReference Include="StyleCop.Analyzers" Version="1.1.118" />
  </ItemGroup>
</Project>

In the .editorconfig I have file markup as given above.

Note that when opening the soltution in VS, I have errors for e.g.

Error   SA1508  A closing brace should not be preceded by a blank line. 
Error   SA1505  An opening brace should not be followed by a blank line.    
Error   SA1507  Code should not contain multiple blank lines in a row   

I run dotnet format --fix-analyzers

These are all fixed! :tada: Blank lines are gone.

More conventional spacing, issues e.g. public SomeClass ( string pk , string sk ) was not fixed by this run, so I run dotnet format a second time with no args, and they are fixed, I get public SomeClass(string pk, string sk)

I can also confirm that without the .editorconfig markup to make these errors not warnings, dotnet format --fix-analyzers warn will remove the blank lines.

Thank you.

sharwell commented 3 years ago

@anthony-steele-cko Note that I would not recommend ever setting analyzer diagnostics to error. This setting will impair unit testing and debugging experiences, and make it difficult to distinguish build warnings (like these) from true errors (e.g. forgetting a ;). You can set Treat Warnings as Errors to true specifically for CI builds to avoid letting warnings get merged to the product.

anthony-steele-cko commented 3 years ago

@sharwell I agree: as proof of concept, it works; as a way to remove blank lines using dotnet format, it comes with a huge overhead.

JoeRobich commented 3 years ago

@anthony-steele-cko Glad things are working for you now. To fix both whitespace formatting and analyzer reported issues in the same run, you can use the command dotnet format --fix-whitespace --fix-analyzers warn.

niemyjski commented 9 months ago

This seems like a good thing to have without having to bring in style cop.

ThijmenDam commented 8 months ago

Agree with @niemyjski. This seems like such a basic feature, why rely on StyleCop?

niemyjski commented 8 months ago

@JoeRobich can you please reopen

JoeRobich commented 8 months ago

@niemyjski If the request is to have these empty line formatting options in the box, then it would be best to open a feature request on Roslyn (https://github.com/dotnet/roslyn/issues). I know Roslyn has experimental analyzers for consecutive empty line and some other empty line scenarios (see https://github.com/dotnet/roslyn/pull/50358).

nhuethmayr commented 7 months ago

Shouldn't this be part of the dotnet format whitespace option? Especially considering that dotnet format is quite slow on large solutions. The only way for it to be acceptable as a pre-commit hook is to use the whitespace mode.

sharwell commented 7 months ago

@norberthuethmayr whitespace options for dotnet format are handled by https://github.com/dotnet/roslyn, so this would still be considered external.