-
-
Notifications
You must be signed in to change notification settings - Fork 101
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
dealing with loops without braces #213
Conversation
else if (someLongCondition == someOtherCondition) | ||
DoSomeLongMethodCall111111111111(); | ||
else | ||
DoSomeLongMethodCall1111111111111111111111111111111111111111(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really a point, but I really like padding length by adding more o
s to Long
than adding 1
s to the end.
That way, when I pronounce it in my head as a really loooooooong method, I feel the weight of the function call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was on board with it..... except rider complains about LooooooooooongMethod() being misspelled and seeing things underlined with red triggers my OCD.
How about ending with ________________ like this? It doesn't have the weight, but I think it makes it easier to read without getting distracted by all the extra 11111111111111111111s or the red squiggles
Node.Print(node.Expression) | ||
Doc.Group( | ||
Token.PrintWithoutLeadingTrivia(node.AwaitKeyword), | ||
node.AwaitKeyword.Kind() != SyntaxKind.None ? " " : Doc.Null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered if this whole thing might look better with pattern matching?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pattern matching is still newish to me so I wasn't even thinking about using it here. My thoughts on doing it here after I changed it.
The is not
is more typing, but I don't mind the way it looks and it kinda reads better. Although changing it caused csharpier to break in an ugly way. I'm thinking csharpier should probably break it like so instead
node.AwaitKeyword.Kind() is not SyntaxKind.None
? " " : Doc.Null,
// or maybe
node.AwaitKeyword.Kind() is not SyntaxKind.None
? " "
: Doc.Null,
I read a discussion the other day where someone pointed out that pattern matching is really nice for something like
if (!(someObject is SomeType))
// vs
if (someObject is not SomeType)
I did use pattern matching for gross condition in another issue just now, and I'm starting to drink the pattern matching kool-aid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I said pattern matching, I meant the funky new match expression
" ",
node.AwaitKeyword.Kind() switch {
not SyntaxKind.None => Token.Print(node.ForEachKeyword),
SyntaxKind.None => Token.PrintWithoutLeadingTrivia(node.ForEachKeyword)
},
" ",
But at the time I thought the new instance of the ternary operator was a nested condition. Looking at the code in more detail I see that they are in fact separate arguments to the Doc.Group()
call. Thus, I don't think there's a significantly nicer way of expressing that than your original code.
closes #202
I did this for for, foreach, if/else if, do and while. I can't think of any other places this would be needed.