Page MenuHomePhabricator

[clang-format] C# property formatting can be controlled by config options
ClosedPublic

Authored by jbcoe on Apr 28 2020, 6:21 AM.

Details

Summary

Allow brace wrapping in C# property accessors to be controlled by configuration options.

Add new tests and revert old test results for Microsoft style to their previous state (as intended).

FormatStyle.BraceWrapping.AfterFunction = true; will change automatic property formatting from

Type MyType { get; set }

to

Type MyType
{ get; set }

Diff Detail

Event Timeline

jbcoe created this revision.Apr 28 2020, 6:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2020, 6:21 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
jbcoe retitled this revision from [clang-format] C# property accessor formatting can be controlled by config options to [clang-format] C# property formatting can be controlled by config options.Apr 28 2020, 6:24 AM
jbcoe edited the summary of this revision. (Show Details)
jbcoe edited the summary of this revision. (Show Details)Apr 28 2020, 6:27 AM
jbcoe edited the summary of this revision. (Show Details)
jbcoe edited the summary of this revision. (Show Details)

Do you think we need to support

verifyFormat(R"(//
public class SaleItem {
  public decimal Price
  { 
    get; 
    set; 
   }
})"Style);

Is that currently possible, and what if users actually want?

public string Host { set; get; }

Do you think we need an option to support variations?

I'm curious why we don't go the opposite direction -- why are even those two styles needed separately?

public int Style1 { get; set }
// vs.
public int Style2
{ get; set }

I'm sure there is a good reason; part of this is to make sure we document it so we may consistently do more updates and support more complicated cases in the same spirit.
IMO starting with some reasonable formatting, and later supporting different styles as needed, has been practical to keep clang-format from becoming harder to maintain.

jbcoe added a comment.EditedApr 28 2020, 9:38 AM

I've just merged this in by mistake. Can revert or amend as we like.

This revision was not accepted when it landed; it landed in state Needs Review.Apr 28 2020, 9:39 AM
This revision was automatically updated to reflect the committed changes.
jbcoe added a comment.Apr 28 2020, 9:42 AM

Reverted this commit. Will pick this up tomorrow. Apologies for the noise!

jbcoe reopened this revision.Apr 28 2020, 9:42 AM
jbcoe added a comment.Apr 29 2020, 3:24 AM
public int Style2
{ get; set }

appears in MS examples https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/properties

public int Style2 { get; set }

is the style we use in our code (where the formatter will be put to immediate use).

Other options can be added/supported (maybe a CSharpPropertyStyle enum?) but I don't think I'm going to have time to devote to that in the immediate future. Happy to review/advise as needed.

public int Style2
{ get; set }

appears in MS examples https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/properties

public int Style2 { get; set }

is the style we use in our code (where the formatter will be put to immediate use).

Other options can be added/supported (maybe a CSharpPropertyStyle enum?) but I don't think I'm going to have time to devote to that in the immediate future. Happy to review/advise as needed.

Maybe consider adding

AfterProperty to the BraceWrapping style?

krasimir accepted this revision.May 6 2020, 10:31 AM

I think this is OK as-is.

@MyDeveloperDay's suggestion of adding AfterProperty to the BraceWrapping style makes sense.
However I think in this case it is natural to use the same style for these properties and for methods (functions) until we have a reason to allow for divergence.
This MS example illustrates how close such "property blocks" match "function body blocks": if you squint, add an () before the property block and sprinkle semicolons inside it, it looks like a method.
The fact that we only need this specifically for C# properties makes it a bit niche.
I'd rather be lazy about BraceWrapping.AfterProperty and add it later, as need arises.

This revision is now accepted and ready to land.May 6 2020, 10:31 AM
MyDeveloperDay accepted this revision.May 12 2020, 12:26 AM

I'm ok with your suggestion if you want to land this.

jbcoe edited the summary of this revision. (Show Details)May 15 2020, 5:54 AM