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

Weird formatting of linq chain #1130

Closed
Rudomitori opened this issue Jan 16, 2024 · 6 comments · Fixed by #1133
Closed

Weird formatting of linq chain #1130

Rudomitori opened this issue Jan 16, 2024 · 6 comments · Fixed by #1133
Milestone

Comments

@Rudomitori
Copy link
Contributor

Input:

var something_________________________________________ = x.Answers
    .Select(y => (DateTime?)y.CreatedAt)
    .Min();

Output:

var something_________________________________________ = x.Answers.Select(y =>
    (DateTime?)y.CreatedAt
)
    .Min();

Expected behavior:

var something_________________________________________ = x.Answers
    .Select(y => (DateTime?)y.CreatedAt)
    .Min();

// or
var something_________________________________________ = x
    .Answers.Select(y => (DateTime?)y.CreatedAt)
    .Min();

// or
var something_________________________________________ = x
    .Answers
    .Select(y => (DateTime?)y.CreatedAt)
    .Min();
@loraderon
Copy link
Contributor

I think this is the same issue as #1079

@belav
Copy link
Owner

belav commented Jan 17, 2024

This seems related to #1079, but this particular case did work quite a ways back. I was having a bit of trouble tracking down how exactly the regression was introduced so went back quite a few versions.

This appeared in 0.26.7 and I was able I track down the source of the problem. The changes in #967 were somewhat dependant on this single line I reverted in #1074. I still need to find a solution that makes all of the updated test cases happy.

Side note - running multiple versions of the playground would be nice to track down regressions like this.
Or maybe a utility that uses docker to show the changes over versions for some code. But I don't know that this comes up often enough to be worth the effort.

@shocklateboy92
Copy link
Collaborator

Side note - running multiple versions of the playground would be nice to track down regressions like this.
Or maybe a utility that uses docker to show the changes over versions for some code. But I don't know that this comes up often enough to be worth the effort.

I think git bisect is what you're after:
https://git-scm.com/docs/git-bisect

Especially since you can write a trivial test and a script to repro it, you can even have it run/find the commit automatically:
https://git-scm.com/docs/git-bisect#_bisect_run

@jods4
Copy link

jods4 commented Jan 19, 2024

A change was recently published that keeps method calls on same line as property access, for stuff like this (look at .ReadFrom., and .Enrich.):

// Formatted with 0.27
builder.UseSerilog(
    (context, logConfig) =>
        logConfig
            .ReadFrom.Configuration(context.Configuration)
            .Enrich.FromLogContext()
            .Enrich.WithExceptionDetails(
                new DestructuringOptionsBuilder().WithDefaultDestructurers()
            )
);

(Unrelated aside: @belav I think the code above would look better if the lambda parameters were kept on the UseSerilog(.. line. It's one more example of the recurring "single parameter lambda" theme.)

But I also noticed that in some cases CSharpier keeps the first .Property.Method(.. of a chain on the inital line, like OP.
I'd agree it's not best for readability and for long chains I'd rather always break and indent at the first property.

Here's another test case taken from my project.
The part entityType / .Keys.Select(k =>..) / .ToList() is ok in my opinion.
The part k.KeyAttributes.Select(f => ..) / .ToList() I don't like and would like a break after k so that all subsequent calls are on the same indentation, like entityType. The worst aspect of this formatting is that closing )) from .Select( are less indented than following .ToList() and it just looks weird / not very readable.

// Formatted with 0.27
var AlternateKeys =
    entityType.Keys.Count > 0
        ? entityType
            .Keys.Select(k => new KeyTemplate
            {
                Name = k.DisplayName.GetDisplayName(),
                ClassName = AddSuffix(k.DisplayName.GetFriendlyName("<missing key name>"), "Key"),
                Fields = k.KeyAttributes.Select(f => new KeyValuePair<string, PropertyTemplate>(
                    f,
                    Properties.SingleOrDefault(x => x.LogicalName == f)
                ))
                    .ToList(),
            })
            .ToList()
        : null;

@belav
Copy link
Owner

belav commented Jan 21, 2024

@shocklateboy92 thanks for the link, that would have helped make what I did a bit easier!

@jods4 I happened to make changes that I believe take care of your second example.
When the initial identifier is <= 4 characters, there was logic to keep the first group of the chain on the same line.
Its main goal was to prevent formatting like this from occurring.

x
    .CallMethod____________________________________________________________()
    .CallMethod____________________________________________________________();

That logic is tweaked a bit, and results in your example like so.

// Formatted with 0.27.1
var AlternateKeys =
    entityType.Keys.Count > 0
        ? entityType
            .Keys.Select(k => new KeyTemplate
            {
                Name = k.DisplayName.GetDisplayName(),
                ClassName = AddSuffix(k.DisplayName.GetFriendlyName("<missing key name>"), "Key"),
                Fields = k
                    .KeyAttributes.Select(f => new KeyValuePair<string, PropertyTemplate>(
                        f,
                        Properties.SingleOrDefault(x => x.LogicalName == f)
                    ))
                    .ToList(),
            })
            .ToList()
        : null;

@belav belav added this to the 0.27.1 milestone Jan 21, 2024
shocklateboy92 pushed a commit that referenced this issue Jan 22, 2024
* Adding test case and tracking the code that caused the problem

closes #1130

* Fixing regression with invocations

* Fixing some edge cases

* One last edge case

* format file
@jods4
Copy link

jods4 commented Jan 22, 2024

Looks good to me!

@belav Aside: entityType.Keys.Count > 0 is not kept on the same line as var AlternateKeys = because of the Rectangle Rule?
I feel like Conditionals are one more instance where it would just look better if we broke it.
IMHO it creates 2 almost empty lines, increases indentation and doesn't really improve readability.

alanssitis pushed a commit to alanssitis/csharpier that referenced this issue Jan 22, 2024
…#1133)

* Adding test case and tracking the code that caused the problem

closes belav#1130

* Fixing regression with invocations

* Fixing some edge cases

* One last edge case

* format file
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

Successfully merging a pull request may close this issue.

5 participants