-
-
Notifications
You must be signed in to change notification settings - Fork 320
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
Move PNC apply to separate Expr/Pattern variant #7480
Conversation
Ready for re-review |
7ab2ac2
to
898b3f5
Compare
), | ||
Apply(&'a Loc<Pattern<'a>>, &'a [Loc<Pattern<'a>>]), | ||
|
||
PncApply(&'a Loc<Pattern<'a>>, Collection<'a, Loc<Pattern<'a>>>), |
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.
Why use a Collection
instead of just a slice?
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.
That was the whole point of the change. Collections handle trailing comments and have uniform formatting
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.
Okay, fair. We can leave this for now, and presumably in the future this won't be a problem since Apply
is going away.
: f | ||
1( | ||
i, | ||
p, # |
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.
This seems like it's formatted incorrectly to me. Either we should move the comments above the 1
line, or keep it after the closing parens.
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 have to guess that this is consistent with what we do for all collections (lists, tuples, records)...
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.
If that's the case, we can make a GitHub issue and try to fix that at some point
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'm okay with ignoring for now if we do something like that in order to avoid leaving this in
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.
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.
Looking back at this, I don't see how that is not the right formatting, here's the original:
1(i,p#
):f
n
and it is being formatted as:
1(
i,
p, #
) : f
n
To me, the comment is obviously meant for the last arg p
and since the comment there forces this to be multi-line, that causes the args to be outdented. This feels like the correct formatting to me.
|
||
# |
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.
Seems like another case that a comment sneaks inside of parens incorrectly.
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.
The original is:
1(ts((0
)
#
)
)f:i7f
e
which if I formatted this in my brain (which is hard because this is nonsense):
1(
ts(
0,
#
),
)
f : i7f
e
which is pretty close to what it does:
1(
ts(
0,
#
),
)
f : i7f
e
With migration it should be:
1(
ts(
0,
#
),
)(f) : i7f
e
Why?
1(...) f
is a PNC apply as the func in a WS apply. With migration we would turn the outer WS apply in a PNC apply.- The
0
inside ofts(...)
has a newline inside the ParensAround, and then another newline in SpacesAfter before the comment. That's the reason for the blank in between after we drop the unneeded ParensAround.
And that's really it.
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 think this mostly closely respects the vertical whitespace requested by the user (without being excessive), while indenting and separating terms correctly - as well as making it easy to add a new arg to both 1(..)
and ts(..)
by adding the trailing commas when outdented.
No description provided.