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

Use common logic for printing rhs of assignment expressions #324

Merged
merged 4 commits into from
Jun 22, 2021

Conversation

dishuk13
Copy link
Contributor

Updates #37

Copy link
Owner

@belav belav left a comment

Choose a reason for hiding this comment

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

Looks good, just the potential change to put the brace on a new line for collection initializers that break.

@@ -62,8 +62,7 @@ class ClassName
var collectionInitializerExpressions = new SomeObject
{
ThisIsACollection = { one, two },
ThisIsAlsoACollection =
{
ThisIsAlsoACollection = {
Copy link
Owner

Choose a reason for hiding this comment

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

I think we want to keep the old formatting on this, but your change does get this consistent with arrayInitializerWithoutSize above it. It could also be a new issue.

node.Right is InitializerExpressionSyntax ? Doc.Null : " ",
Node.Print(node.Right)
)
RightHandSide.Print(node.Right)
Copy link
Owner

Choose a reason for hiding this comment

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

👍 for reusing the code from VariableDeclaration in here

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.

More pattern matching magic can make this code more magical... 🧙‍♂️ 🌟

or LambdaExpressionSyntax
or AwaitExpressionSyntax
or WithExpressionSyntax
? FormatMode.NoIndent
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like a nested ternary if on type matches. Usually, a switch statement looks better.

Not sure how good it will look with a dozen ors. But if it doesn't look good you can probably split them into multiple arms of the switch.


private static Doc IndentIfNeeded(ExpressionSyntax initializerValue, FormatMode formatMode)
{
if (formatMode is FormatMode.NoIndent)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this whole function could be replaced by an expression bodied method with a single switch statement. Up to you if you think this code looks better though.

@dishuk13 dishuk13 requested review from shocklateboy92 and belav June 20, 2021 09:34
Copy link
Owner

@belav belav left a comment

Choose a reason for hiding this comment

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

👍

public static Doc Print(ExpressionSyntax node)
{
var groupId = Guid.NewGuid().ToString();
return node switch
Copy link
Owner

Choose a reason for hiding this comment

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

This gives us another good test case for #237.

@shocklateboy92 shocklateboy92 merged commit 726c6bf into belav:master Jun 22, 2021
@dishuk13 dishuk13 deleted the rhs-expression branch June 22, 2021 20:39
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.

3 participants