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.

Sign up for GitHub

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

Jump to bottom

Getting element access to break more consistent in long invocation chains. #967

Merged
merged 7 commits into from
Oct 10, 2023
Merged

Getting element access to break more consistent in long invocation chains. #967

merged 7 commits into from
Oct 10, 2023

Conversation

belav
Copy link
Owner

@belav belav commented Sep 29, 2023

closes #956

@belav
Copy link
Owner Author

belav commented Sep 30, 2023

This had a number of side effects I need to review. At first glance some of these changes are for the better, but some we maybe don't want.

https://github.com/belav/csharpier-repos/pull/89/files

@belav belav marked this pull request as ready for review October 1, 2023 20:59
@belav
Copy link
Owner Author

belav commented Oct 1, 2023

This morphed a bit, and turned into getting properties to break more consistently with invocations and array access.
I believe there are still a couple things to improve, but this does resolve some of the issues in #451

@shocklateboy92
Copy link
Collaborator

shocklateboy92 commented Oct 3, 2023 •
edited
Loading

So uh... is this ready for review now?

Edit: I know I'm procrastinating on #923 , but I promise I'll get to it soon™ 😉

@belav
Copy link
Owner Author

belav commented Oct 3, 2023 •
edited
Loading

So uh... is this ready for review now?

Whoops, yeah!

I believe there are still a couple things to improve, but this does resolve some of the issues in #451

What I meant by this wasn't really clear after I reread it. I meant that this PR takes care of parts of #451, but not enough that #451 can be closed.

shocklateboy92
shocklateboy92 approved these changes Oct 10, 2023
View reviewed changes
Copy link
Collaborator

@shocklateboy92 shocklateboy92 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM.

I really like how this makes it much less likely for array indexers to break.

Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChain_PropertiesConsistent.test

// TODO too hard to change this for now, will do it in https://github.com/belav/csharpier/issues/451
var someVariable = this.Property.CallMethod(
someValue => someValue.SomeProperty == someOtherValue___________________________
Copy link
Collaborator

Choose a reason for hiding this comment

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

also this isn't all that bad.

@shocklateboy92 shocklateboy92 merged commit 4e4bb4d into main Oct 10, 2023
3 checks passed
@shocklateboy92 shocklateboy92 deleted the array-access-break branch October 10, 2023 13:35
@Jackenmen Jackenmen mentioned this pull request Nov 26, 2023
Closed
@belav belav mentioned this pull request Nov 28, 2023
Closed
@belav belav mentioned this pull request Jan 17, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shocklateboy92 shocklateboy92 shocklateboy92 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Seems like incorrect break for chained property and method calls
2 participants
@belav @shocklateboy92