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

Remove prettier style method chaining #1313

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

flostellbrink
Copy link

I was experimenting with method chaining, and noticed that by removing GroupPrintedNodesPrettierStyle and only using GroupPrintedNodesOnLines formatting improved in my opinion.

Specifically this fixes #1298 and didn't seem to have any significant negative effects.

This seems a bit too easy, I'm sure there's a reason for the existing setup.
But I thought I'd bring it up at least.

Copy link
Collaborator

@shocklateboy92 shocklateboy92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm. Since this is a style change, I'll wait on @belav to weigh in.

He normally does stuff for this project on Mondays (depending on time availability).

@belav
Copy link
Owner

belav commented Aug 4, 2024

I ran it on all of this code - https://github.com/belav/csharpier-repos/pull/113/files - to see some real world examples.

I think the first change in there is why this code sticks around. I believe there is a lot of EF or aspnetcore code that has a lot of Property.CallMethod().Property.CallMethod() type chains.

// current
httpResponseException = aggregateException
    .Flatten()
    .InnerExceptions.Select(ExtractHttpResponseException)
    .Where(ex => ex != null && ex.Response != null)
    .OrderByDescending(ex => ex.Response.StatusCode)
    .FirstOrDefault();

// this PR
httpResponseException = aggregateException
    .Flatten()
    .InnerExceptions
    .Select(ExtractHttpResponseException)
    .Where(ex => ex != null && ex.Response != null)
    .OrderByDescending(ex => ex.Response.StatusCode)
    .FirstOrDefault();

I do have a ticket somewhere, to prefer keeping method calls together. Which this does kind of accomplish. Perhap the logic for when to use the prettier style grouping just needs to be adjusted.

// current
IExtensibleModelBinder childBinder = bindingContext.ModelBinderProviders.GetBinder(
    controllerContext,
    innerBindingContext
);

// this pr
 IExtensibleModelBinder childBinder = bindingContext
    .ModelBinderProviders
    .GetBinder(controllerContext, innerBindingContext);

// maybe should try to keep properties together like this
 IExtensibleModelBinder childBinder = bindingContext.ModelBinderProviders
    .GetBinder(controllerContext, innerBindingContext);

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 this pull request may close these issues.

Formatting Changes in Invocation Chains After Version 0.27.1
3 participants