csharpier icon indicating copy to clipboard operation
csharpier copied to clipboard

[Question] - Can an empty class format be skipped without applying the 'ignore comment'? See image example

Open jeffward01 opened this issue 1 year ago • 16 comments

Hello all!

After reading the configuration document - I do not think this is possible, but I still wanted to ask.

CSharpierPlayground

The goal is:

  • I do not want empty classes to be formatted. (It causes issues for some Code Generation tools I have)
  • I do not want to apply the // csharpier-ignore to each empty class
  • I would like to add a value in a configuration file to tell csharpier to skip this format

Is this currently possible? (I know csharpier has no plans for future configuration options)

Thanks!

jeffward01 avatar Aug 28 '22 17:08 jeffward01

It is not currently possible to configure, but I have been considering changing the formatting of empty blocks.

There are times where I create an empty method and as I think about what I want to put in the method I hit ctrl-s. That formats the method to a single line and screws with my cursor placement.

Empty blocks don't really occur that often in real code so I don't think the extra lines will really cause any headaches.

Out of curiosity, what code generation tools are you using that have issues with the collapsed blocks? I'm debating between these too, and would lean towards the first one assuming it works with the code generation tools and solves the problem I have.

class ClassName
{
}

class ClassName
{

}

belav avatar Sep 04 '22 21:09 belav

If I'm not mistaken, there are editoconfig settings for these brackets, like csharp_new_line_before_open_brace.

https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/code-style-rule-options

It would be good for the tool to respect relevant editorconfig rules in general.

Meligy avatar Sep 05 '22 00:09 Meligy

@Meligy if CSharpier respected editorconfig rules it would essentially be allowing configuration. There are no plans to allow any configuration beyond the current limited set of options. There is a backlog issue for reading the supported configuration from an editorconfig.

belav avatar Sep 06 '22 16:09 belav

personally, i hope u dont change this

Pentadome avatar Sep 27 '22 10:09 Pentadome

personally, i hope u dont change this

@Pentadome - Why is this? It could perhaps be a flag such as --csharp_new_line_before_open_brace which would create classes like:

class ClassName
{

}

@belav - I also respect that this is venturing into the territory of opening a can of worms, where the developer could say:

Well, one rule XYZ from .editorconfigis respected, lets add 2 more.. then 10 more, actually, lets use the.editorconfig as a possible source of truth.

It would be cool, but I do understand that this opens the potential for deeper change which is not quiet in the spirit of csharpier, although there is a backlog for it.

Just wanted to say big thanks for the library @belav <3

jeffward01 avatar Sep 29 '22 16:09 jeffward01

@belav, my 2 cents, as somebody who recently started using csharpier and is very much enjoying it. I greatly appreciate that there are no options, and would very much like the editorconfig can of worms to remain shut.

Empty blocks don't really occur that often in real code so I don't think the extra lines will really cause any headaches.

There are use cases for empty methods and classes. I am not saying all the examples below are good ideas, but there are not so unusual to see in the wild.

  • using classes or interfaces as "marker" for a bit of extra type safety:
class Message { }

// then use as a constraint
class SomeContainer<TMessage> where TMessage : Message
{
    // or
    void DoSomething<TMessage>(TMessage) where TMessage : Message
}

// define the message
class MyMessage : Message
{
    /* some stuff here */
}
  • inheriting classes just to differentiate them
class MyClass : BaseClass { }
class NotQuiteMyClass : BaseClass { }
  • extending multiple interfaces
interface IMyInterface : IFoo, IBar { }
  • making a class or interface non-generic
interface IInterfaceOfMyClass : IFoo<MyClass> { }
class MyDerivedClass : BaseClass<MyClass> { }
  • overriding a virtual method, or implementing an interface method with a no-op
override void TheMethod() { }

C# is already a language that takes a lot of vertical space. The ratio of useful code lines (not empty or just a bracket) to total lines is not very high.

// 1 line - with meaninful content
class Message { }

// 3 lines - 66% of the lines are blank - but still fairly compact
class Message
{
}

// 4 lines - 75% of the lines are blank - please avoid that
class Message
{

}

Regarding the inconvenience of the cursor ending up in the wrong place with saving an empty class when you intend to edit it further: you can add // to the body and csharpier will keep the brackets on separate lines.

public class ClassName
{
    //    <- temporary
}

Regarding code generation tools, what is the problem exactly? Is it (1) that the codegen tool malfunctions when the code was formatted by csharpier, or (2) that it outputs code that is not formatted the csharpier way?

If it is (1), then it feels wrong to change csharpier output for everyone because some codegen tool is buggy. Also, as pointed out, csharpier offers various ways to ignore code or entire files (https://csharpier.com/docs/Ignore) that should be sufficient to ignore those files.

If it is (2), the solution is to run csharpier after the codegen tool.


Once again, thanks for csharpier :)

gat-cs avatar Oct 26 '22 07:10 gat-cs

Just wanted to chime in that the current format really disrupts stubbing out classes and methods. I run into this more often than I expected, so I support changing the empty body formatting to be

class-or-method-declaration
{
}

domn1995 avatar Nov 11 '22 20:11 domn1995

I did a quick version of these changes in https://github.com/belav/csharpier/pull/763 as a way to see what kind of affect this would have on some codebases.

One thing I ran into was if empty lambdas should also include a new line. I left them with a new line to see some better real world code that would be affected.

private static readonly Action _callbackCompleted = () => { };
// vs what is currently in the PR
private static readonly Action _callbackCompleted = () => {
};

Running this across all of csharpier-repos affected at least 5,000 (github won't show the real count past 5k) of the 71,000 files. The review page for the PR just crashes. My next step is figuring out a way to review these changes, either splitting apart the PR into smaller chunks or seeing if I can review it locally somehow.

For what it's worth, prettier seems somewhat inconsistent about empty blocks and I couldn't find any discussion about it.

if (true) {
} else {
}

function Method() {}

class Class {}

while (true) {}

for (var x = 1; x < 2; x++) {}

belav avatar Nov 23 '22 18:11 belav

Thank you for checking this out, thats alot of changes, I agree with what you did with the lambda, also - it makes it look more like a 'method' rather than a property, which for me is easier to read that it has function, its not just a delegate, its an invoked method action that performs 'work' on something.

I agree with your decisions! really cool work!

jeffward01 avatar Nov 24 '22 15:11 jeffward01

I got this running against the aspnetcore portion of csharpier-repos.

I don't think formatting empty lambdas with a line break is a good change. It results in things like the following.

builder.AddJwtBearer(jwtBearerScheme, o => { });

// becomes

builder.AddJwtBearer(
    jwtBearerScheme,
    o =>
    {
    }
);
builder.AddOpenIdConnect(openIdConnectScheme, null, o => { });
builder.AddCookie(cookieScheme, null, o => { });

// becomes 

builder.AddOpenIdConnect(
    openIdConnectScheme,
    null,
    o =>
    {
    }
);
builder.AddCookie(
    cookieScheme,
    null,
    o =>
    {
    }
);

When excluding empty lambdas, ~17% of the files in aspnetcore are modified. That's quite a bit more than I was expecting.

Thinking about it more, I think CSharpier should be focused on formatting to a state that is most readable. I don't think that adding a line break to an empty block makes it more readable. The empty block collapsing when stubbing out methods and saving them before filling it in can be annoying, but maybe the plugins can do a better job of keeping the cursor inside of the block. Everyone that uses the plugin learning to add // to them temporarily isn't ideal, but is the other solution. @jeffward01 I'm also curious about the code generation tools that have issues with the collapsed empty blocks. Maybe they can be modified to work with the collapsed blocks?

belav avatar Nov 27 '22 16:11 belav

@belav - They can absolutely be modified to work with empty blocks, the tools I am referring to were a bit more rigid initially, however now they use Roslyn compiler to compile the code as a string, then walk the syntax tree.

So for my use-case at lease the block { ... } is more of a style / formatting issue and dealing with .editorconfigs. Such as this example:

  • Some code generation occurs, or perhaps you have a task on Azure Pipelines that 'cleans' the code, and conforms to a style.
  • It's a bit heavy to install Resharper CLI, although we can / have done that. But its not as 'simple' as Csharpier due to Jetbrains config files
  • We will run 'clean-up' with Csharpier and it will format the code. However...
  • When we open the code again at our workstatations as an example, there are formatting errors everywhere due to the editorconfig. most of the errors have to do with the { ... } because many teams dislike the Java or Javascript style of braces, so they enforce a line break.

Tbh its a small issue and not a deal breaker if its just that. But this leads me to my next point, and perhaps other will agree:

I think CSharpier should be focused on formatting to a state that is most readable. I

I totally agree with this. I think let's keep it simple, and do simple very very well, or open up a can of worms and do complex realy really well. Such as 'fix namespaces', query files that have namespace errors, etc. Things that you can do in an IDE, but not with Resharper CLI as an example.

But regarding keeping it simple:

Example 1

public record Student { }

Example 2

public record Student
{
}

My eyes are trained to look for Example 2, just because as a C# dev, that's always what we've done. So file with 100+ lines of code, my eyes have missed the Example 1 fornatting, and I have to look closer to see it. Its more of just an intuitive thing and personal perference. So idk. Just want to share my 2 cents.


When excluding empty lambdas, ~17% of the files in aspnetcore are modified. That's quite a bit more than I was expecting.

That's quite a bit, I agree. From what we know the aspcore team uses standard 'clean-up', so I would take a venture to guess that we should try to aim for 0.00% of code changes, if this makes sense.


Question:

Question for you about implementation:

Sorry, im not quite sure how CSharpier formats the code, whether if its Roslyn based, or some text parser, but would it be possible to detect if the code is a Lambda, or a record / struct / class and selectively format?

I worked with this project for a bit: https://github.com/aws/codelyzer

It's pretty cool, it analyzes the syntax in a very easy way, and you can easily parse it to Json or whatever you need. I only suggest this as an example of something that can identify lambda's versus classes as an example.

Other suggestion

Maybe it would be better to allow third-party devs to write 'plugins' for csharper sorta like an .editorconfig. So Csharpier would focus on doing simple very very well, and the plugins can modify the code and introduce new features as needed?

jeffward01 avatar Nov 28 '22 13:11 jeffward01

Just so you have some context too, some of my general use-cases of csharpier is this:

  • Code generators, and I don't want to work with Resharper, perhaps I generate (1) file like GetStudentByIdHandler.cs and I only want to clean up that one file, and not the others.

  • Build steps: Use csharpier instead of resharper-cli during bluild and clean up because its easier to work with. This has its drawbacks regarding .editorconfig, but its still a nice 'sanity check'.

  • In large projects, something using the default cleanup by VS or Rider, is very very slow. I have found myself opening up a command line instance and running csharpier to expedite cleanup

  • I use the SDK verion (non-command line) of csharpier in most of the code generators I work with. its super super super nifty and probably my favorite feature about csharpier.


I hope that these are helpful!!! Thanks for all the exploration and work on the { ... } 🚀 Please let us know how we can be helpful to you!

jeffward01 avatar Nov 28 '22 13:11 jeffward01

  • Build steps: Use csharpier instead of resharper-cli during bluild and clean up because its easier to work with. This has its drawbacks regarding .editorconfig, but its still a nice 'sanity check'.

Sorry to hijack the thread but is there any way that the Resharper CLI can be used in MSBuild build, not having to be run as a separate CI config task etc.?

Meligy avatar Nov 28 '22 14:11 Meligy

can be used in MSBuild build, not having to be run as a separate CI config task etc.?

Not natively. I mean resharper cli is defintely not apart of MSBuild. You'd need to use a config task which tbh is pretty easy to do, for a cli tool at least. It's much easier than configuring test reporting and stuff like that.

I suppose you can use something like NUKE Build here, its pretty cool: https://nuke.build/docs/common/cli-tools/

Don't use Cake Build - its a mess.

It also ties in with MsBuild - rather actually, its a wrapper ontop of MsBuild.

image

Resharper is supported.

Resharper CLI: https://www.jetbrains.com/help/resharper/ReSharper_Command_Line_Tools.html

We've always done something like these solutions:

  • https://stackoverflow.com/questions/72962641/how-to-ensure-a-dotnet-tool-is-installed-in-an-azure-pipeline
  • https://stackoverflow.com/questions/58905151/install-run-custom-dotnet-tool-on-azure-devops-release

Use this script as a task to play around with your directory structure:


# Prints the directory structure to the output of the Pipeline build.
# Prints from root and all nested children

- task: CmdLine@2
  displayName: 'print directory structure'
  inputs:
    script: |
      echo "Structure of work folder of this pipeline:"
      tree $(Agent.WorkFolder)\1 /f
      
      echo "Build.ArtifactStagingDirectory:" 
      
      echo "$(Build.ArtifactStagingDirectory)"
      
      echo "Build.BinariesDirectory:" 
      
      echo "$(Build.BinariesDirectory)"
      
      echo "Build.SourcesDirectory:"
      
      echo "$(Build.SourcesDirectory)"

Personally I think if the only think your doing is running a cleanup task, Nuke is overkill for that, just use tasks in azure devops

jeffward01 avatar Nov 28 '22 15:11 jeffward01

Would a "magic" workaround like only using linebreaks if there is a single class/interface in the entire file work?

Class1.cs

public class Class1
{

}

Classes.cs

class MyClass : BaseClass { }

class NotQuiteMyClass : BaseClass { }

loraderon avatar Dec 14 '23 10:12 loraderon

Would a "magic" workaround like only using linebreaks if there is a single class/interface in the entire file work?

I really like this idea!

belav avatar Dec 28 '23 20:12 belav