Inconsistent breaking with lambdas with/without parens

[belav]

The same call with or without parens ends up breaking differently.

Maybe the 2nd version should be consistent with the first version, and just break before the parameter even when there are no parens.

builder.Services.AddPooledDbContextFactory<CommerceContext>(
    (options) =>
    {
        if (enableLazyLoading)
        {
            options.UseLazyLoadingProxies();
        }

        options.UseSqlServer(
            ConnectionStringProvider.Current.ConnectionString,
            o => o.UseQuerySplittingBehavior(QuerySplittingBehavior.SplitQuery)
        );
        if (builder.Environment.IsDevelopment())
        {
            options.EnableSensitiveDataLogging();
        }
        options.UseModel(
            Insite.Data.Providers.EntityFramework.CompiledModels.CommerceContextModel.Instance
        );
    }
);

builder.Services.AddPooledDbContextFactory<CommerceContext>(options =>
{
    if (enableLazyLoading)
    {
        options.UseLazyLoadingProxies();
    }

    options.UseSqlServer(
        ConnectionStringProvider.Current.ConnectionString,
        o => o.UseQuerySplittingBehavior(QuerySplittingBehavior.SplitQuery)
    );
    if (builder.Environment.IsDevelopment())
    {
        options.EnableSensitiveDataLogging();
    }
    options.UseModel(
        Insite.Data.Providers.EntityFramework.CompiledModels.CommerceContextModel.Instance
    );
});

[jods4]

If there is a single parameter, I like the second case better.

If there are more than one parameter, I think the first case is better, otherwise indentation between parameters is inconsistent.

I was about to post the following cases as a new issue about this, but I guess it kind of fits in this issue as well?:

public class ClassName
{
    // Fits on 1 line, ok
    public object Test1()
    {
        return table.Where(t => t.SomeLongColumn == 1 || t.OtherLongColumn == 2);
    }

    // Fits on 2 lines, ok
    // Alternative: "table.Where(t =>" would be ok, too
    public object Test2()
    {
        return table.Where(
            t => t.SomeLongColumn == 1 || t.OtherLongColumn == 2 || t.SomeLongColumn == 3
        );
        // return table.Where(t => 
        //     t.SomeLongColumn == 1 || t.OtherLongColumn == 2 || t.SomeLongColumn == 3
        // );
    }

    // Feels like there's one wasted "t =>" line
    // Would like "table.Where(t =>" better, as long as there's a single argument
    // and the lambda parameters fit on the first line
    public object Test3()
    {
        return table.Where(
            t =>
                t.SomeLongColumn == Guid.Parse("a112c293-5a3a-ed11-9db1-000d3a24f3d4")
                || t.OtherLongColumn == Guid.Parse("a112c293-5a3a-ed11-9db1-000d3a24f3d4")
        );
        // return table.Where(t =>
        //     t.SomeLongColumn == Guid.Parse("a112c293-5a3a-ed11-9db1-000d3a24f3d4")
        //     || t.OtherLongColumn == Guid.Parse("a112c293-5a3a-ed11-9db1-000d3a24f3d4")
        // );
    }

    // More than 1 parameters would look weirdly indented as suggested in Test3,
    // probably best to stick to strictly one parameter per line
    public object Test4()
    {
        return table.Where(
            t =>
                t.SomeLongColumn == Guid.Parse("a112c293-5a3a-ed11-9db1-000d3a24f3d4")
                || t.OtherLongColumn == Guid.Parse("a112c293-5a3a-ed11-9db1-000d3a24f3d4"),
            u => u.One + u.Two,
            42
        );
    }
}

I think Test3 would benefit from putting the lambda on same line as Where and de-indent its body one level.
As mentionned, if there are multiple parameters it's probably best not to do that (see Test4).

For consistency, I think Test2 could apply the same treatment (t => on previous line), which leaves more room for the lambda on next line and avoids breaking the lambda parameter alone on a single line.

Should CSharpier automatically remove brackets around single lambda parameters? I think Prettier does it or it might be one of the few Prettier options? I personally prefer to avoid parameter brackets when they are not required.

[belav]

I was about to post the following cases as a new issue about this, but I guess it kind of fits in this issue as well?

Yeah I think it fits here.

I'm not sure how I feel about Test2 and Test3 putting t => onto the same line as .Where

@commonquail linked to https://github.com/google/google-java-format/wiki/The-Rectangle-Rule, and I really liked the idea of it when reading it. I've been trying to keep it in mind with any formatting changes. It isn't a strict rule and I'm not sure if they decided lambda's would be an exception to it.

// this technically violates the rule with the .Where, but seems like a good exception
return table.Where(
    t =>
        t.SomeLongColumn == Guid.Parse("a112c293-5a3a-ed11-9db1-000d3a24f3d4")
        || t.OtherLongColumn == Guid.Parse("a112c293-5a3a-ed11-9db1-000d3a24f3d4")
);

// the t => violates the rule as well, but saves vertical space. Maybe it depends on the length of t
return table.Where(t =>
    t.SomeLongColumn == Guid.Parse("a112c293-5a3a-ed11-9db1-000d3a24f3d4")
    || t.OtherLongColumn == Guid.Parse("a112c293-5a3a-ed11-9db1-000d3a24f3d4")
);

// this could satisfy the rule if the || line was indented more, but that could get tricky and potentially waste a lot of space
return table.Where(
    t => t.SomeLongColumn == Guid.Parse("a112c293-5a3a-ed11-9db1-000d3a24f3d4")
        || t.OtherLongColumn == Guid.Parse("a112c293-5a3a-ed11-9db1-000d3a24f3d4")
);

Should CSharpier automatically remove brackets around single lambda parameters? I think Prettier does it or it might be one of the few Prettier options? I personally prefer to avoid parameter brackets when they are not required.

This sounded familiar, and yeah they do have an option for it. They actually default to always adding the ( ). I think I could be onboard with removing them. My gut says most people writing c# normally don't include them, but I would want to validate that before deciding to do it. It does violate the "no modifying syntax" rule that has been around so far. But there have been a few suggestions for things that would start to violate that rule. I'm curious if there is a prettier issue with discussions around why they now default to including them.

[jods4]

While it's a good guiding principle, it should be noted that google-java-format itself makes exceptions to the Rectangle Rule, they mention it explicitly in the page you link (and proceed with some examples):

google-java-format implements a number of exceptions to the Rectangle Rule, but it seems certain that even more might be worthwhile.

Personally, I think it's nice to reduce indentation by one level and not use a full line just for t => when it fits on the previous line. (For the same reason, I like the 2nd example in the original post of this issue more).

I tried to look for some examples in the wild, and on MS C# Coding Conventions I found these two examples, that are not function parameters but similar (and both violate the Rectangle Rule):

// In Delegates section:

public static Action<string, string> ActionExample2 = (x, y) =>
    Console.WriteLine($"x is: {x}, y is {y}");

// In Event handling section:
// (Personally, I would de-indent the { } block to be consistent with methods formatting):

public Form2()
{
    this.Click += (s, e) =>
        {
            MessageBox.Show(
                ((MouseEventArgs)e).Location.ToString());
        };
}

[commonquail]

it should be noted that google-java-format itself makes exceptions to the Rectangle Rule

Exceptions to application of the rule; not inconsistency in the application of the rule to the same syntactical construct (for certain definitions of consistency).

Personally, I think it's nice to reduce indentation by one level and not use a full line just for t => when it fits on the previous line.

That has to be balanced with induced churn, too. Formatting exceptions that alter the vertical placement of constructs is particularly susceptible to pointless churn (spacing to align columns across line is a good example of a waste of time).

It's worth keeping in mind that lambdas are syntactically trash, too; they work very poorly with any formatting rules, especially automated ones, and it is often pragmatically preferential to leverage method references to mitigate pathological layouts (and also because pathological cases are frequently complex enough to warrant formalization).

Anyway, for posterity here is 4SP-indented google-java-format 1.17 on some of the samples from this thread and four novel samples: Test3b, Test3c, and Form2b showing alternate forms, and Test5 showing obnoxious lambda parameters:

$ java  -jar google-java-format-1.17.0-all-deps.jar --aosp A.java 

class A {
    void a() {
        builder.Services.<CommerceContext>AddPooledDbContextFactory(
                (options) -> {
                    if (enableLazyLoading) {
                        options.UseLazyLoadingProxies();
                    }

                    options.UseSqlServer(
                            ConnectionStringProvider.Current.ConnectionString,
                            o -> o.UseQuerySplittingBehavior(QuerySplittingBehavior.SplitQuery));
                    if (builder.Environment.IsDevelopment()) {
                        options.EnableSensitiveDataLogging();
                    }
                    options.UseModel(
                            Insite.Data.Providers.EntityFramework.CompiledModels
                                    .CommerceContextModel.Instance);
                });

        builder.Services.<CommerceContext>AddPooledDbContextFactory(
                options -> {
                    if (enableLazyLoading) {
                        options.UseLazyLoadingProxies();
                    }

                    options.UseSqlServer(
                            ConnectionStringProvider.Current.ConnectionString,
                            o -> o.UseQuerySplittingBehavior(QuerySplittingBehavior.SplitQuery));
                    if (builder.Environment.IsDevelopment()) {
                        options.EnableSensitiveDataLogging();
                    }
                    options.UseModel(
                            Insite.Data.Providers.EntityFramework.CompiledModels
                                    .CommerceContextModel.Instance);
                });
    }
}

$ java  -jar google-java-format-1.17.0-all-deps.jar --aosp ClassName.java 

public class ClassName {
    public object Test1() {
        return table.Where(t -> t.SomeLongColumn == 1 || t.OtherLongColumn == 2);
    }

    public object Test2() {
        return table.Where(
                t -> t.SomeLongColumn == 1 || t.OtherLongColumn == 2 || t.SomeLongColumn == 3);
    }

    public object Test3() {
        return table.Where(
                t ->
                        t.SomeLongColumn == Guid.Parse("a112c293-5a3a-ed11-9db1-000d3a24f3d4")
                                || t.OtherLongColumn
                                        == Guid.Parse("a112c293-5a3a-ed11-9db1-000d3a24f3d4"));
    }

    public object Test3b() {
        return table.Where(
                t -> {
                    return t.SomeLongColumn == Guid.Parse("a112c293-5a3a-ed11-9db1-000d3a24f3d4")
                            || t.OtherLongColumn
                                    == Guid.Parse("a112c293-5a3a-ed11-9db1-000d3a24f3d4");
                });
    }

    public object Test3c() {
        return table.Where(
                t -> {
                    var guid = Guid.Parse("a112c293-5a3a-ed11-9db1-000d3a24f3d4");
                    return t.SomeLongColumn == guid || t.OtherLongColumn == guid;
                });
    }

    public object Test4() {
        return table.Where(
                t ->
                        t.SomeLongColumn == Guid.Parse("a112c293-5a3a-ed11-9db1-000d3a24f3d4")
                                || t.OtherLongColumn
                                        == Guid.Parse("a112c293-5a3a-ed11-9db1-000d3a24f3d4"),
                u -> u.One + u.Two,
                42);
    }

    public object Test5() {
        return table.Where(
                (t,
                        a,
                        b,
                        c,
                        d,
                        e,
                        f,
                        g,
                        hhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhh) ->
                        t.SomeLongColumn == Guid.Parse("a112c293-5a3a-ed11-9db1-000d3a24f3d4")
                                || t.OtherLongColumn
                                        == Guid.Parse("a112c293-5a3a-ed11-9db1-000d3a24f3d4"));
    }

    public Form2() {
        this.Click +=
                (s, e) -> {
                    MessageBox.Show(((MouseEventArgs) e).Location.ToString());
                };
    }

    public Form2b() {
        this.Click += (s, e) -> MessageBox.Show(((MouseEventArgs) e).Location.ToString());
    }
}

[jods4]

Here is one more real-life example (from the project I'm working on), to illustrate where I think CSharpier is using a lot of lines for little code:

  public async Task<object> GetCountry(Guid id)
  {
    var client = await OpenClient();
    return await client
      .For<Country>()
      .Key(id)
      .Select(
        c =>
          new
          {
            c.Mim_CountryId,
            c.Mim_Name,
            c.Mim_Code
          }
      )
      .FindEntryAsync();
  }

The code is clear, but there are a lot of "single token" lines.

I personally would like breaking the Rectangle rule in more contexts. We discussed the lambda parameter c =>, but I think it would apply equally well for new anonymous object.

Some suggestions:

    // If a method call (.Select) has a single parameter, 
    // don't insert a hard line-break before that parameter:
    return await client
      .For<Country>()
      .Key(id)
      .Select(c => new
      {
        c.Mim_CountryId,
        c.Mim_Name,
        c.Mim_Code
      }) // Non-indented } can go on same line as )
      .FindEntryAsync();

    // For illustration: same code with multiple parameters
    return await client
      .For<Country>()
      .Key(id)
      .Select(  // Hard-break + indent when multiple params
        c => new // No forced line-break between => and new anonymous object
        {
          c.Mim_CountryId,
          c.Mim_Name,
          c.Mim_Code
        },
        42
       ) // Closing ) is on a different line if previous line was indented
      .FindEntryAsync();

IMHO I think that example is significantly nicer than current CSharpier (above), what do you think?

[domn1995]

+1 for following the rectangle rule unless we have a very, very good reason to stray from it. Finding more unrelated tokens on the same line "nicer to read" is not objective enough of a reason, imo.

[jods4]

CSharpier is a formatter, it only changes whitespace. "nicer to read" is the name of the game, there will never be a "very, very good reason". It's purpose is to make code nicer to read and consistent.

The "rectangle rule" is a good first principle but it's not a law.

There are many examples to look at in this thread and decide what's the best looking formatting.

Here's some code formatted by the CSharpier 0.25.0.
I don't think the 2 statements are consistent, so I'd say one should change. @domn1995 what's your pick?

public class C
{
    public Form2()
    {
        this.Click += (s, e) =>
        {
            MessageBox.Show(((MouseEventArgs)e).Location.ToString());
        };

        this.Click.CombineImpl(
            (s, e) =>
            {
                MessageBox.Show(((MouseEventArgs)e).Location.ToString());
            }
        );
    }
}