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

fix: bugs from #1066 #1078

Merged
merged 4 commits into from
Dec 20, 2023
Merged

fix: bugs from #1066 #1078

merged 4 commits into from
Dec 20, 2023

Conversation

Rudomitori
Copy link
Contributor

Fix the first and third bugs described in #1077

Group id used in ArgumentListLikeSyntax was not unique
and the nested lambda overrode the state of the parent lambda
shocklateboy92
shocklateboy92 previously approved these changes Dec 18, 2023
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.

LGTM.

Do you want to merge this as is, and fix the second bug mentioned in #1077 in a separate PR?

Or should I wait for you to push the fix for the second one also into this branch?

@@ -10,14 +10,15 @@ FormattingContext context
)
{
var docs = new List<Doc> { Token.Print(openParenToken, context) };
var lambdaId = Guid.NewGuid();

switch (arguments)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I forgot to mention this in your original PR, but this all branches of this switch statement just add something to the list.
Wouldn't it look better if you used the switch expression instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The expression looks almost the same for me

@Rudomitori
Copy link
Contributor Author

I will fix the second bug in other PR

@Rudomitori
Copy link
Contributor Author

Well, I've fixed the last bug before PR is merged, so I've decided to add the changes in this PR

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.

Cheers!

@shocklateboy92 shocklateboy92 merged commit 43ba8d7 into belav:main Dec 20, 2023
3 checks passed
@Rudomitori Rudomitori deleted the feature/fix_bug_from_1066 branch December 26, 2023 05:17
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.

2 participants