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

align match arms with match_arm_align_threshold #3996

Closed

Conversation

geogriff-signal
Copy link

@geogriff-signal geogriff-signal commented Jan 3, 2020

This is an implementation of match arm alignment as proposed in #894. A previous attempt #1704 stalled out over a year ago. This implementation has a few important differences:

  • Alignment is reset after a match arm with a block body. Subjectively, this makes alignment look better (as shown in the added example in Configurations.md), since a block implies there's at least two lines code between the arms (as empty blocks are collapsed), and having lines interrupting match arms looks.. strange. I have since realized, however, that single-expression bodies circumvent this check, so we may need a different approach if we want this. I want to get your feedback before revisiting this, though.
  • The "threshold greater than max_width forces always alignment" was dropped, as no other threshold configuration parameter has this behaviour.
  • Match arms are rewritten before alignment, which is the correct behaviour.
  • Only the length of the last line of the match arm LHS is considered, as opposed to the max of all lines. This can be seen in the following fragment. Not sure what's the correct behaviour here, so I picked my favorite:
match foo {
    Foo   => (),
    FooBarBaz
    | Baz => (),

Another unanswered question:

  • Alignment is currently not performed at all if any arm causes the number of inserted spaces to exceed the threshold. We could instead align the preceding arms (if they satisfy the threshold) and start a new alignment group. We could also try to find the set of arm groups which end up aligning the most arms, but the naive implementation of that, at least, would be an exponential search.

Also, since I did copy some documentation phrasing and tests from #1704, I'm not sure what attribution / licensing implications that has. I could always try to re-write those.

@geogriff-signal
Copy link
Author

I have since realized, however, that single-expression bodies circumvent this check

This may be irrelevant depending on how #3995 is handled (see this comment)

@topecongiro
Copy link
Contributor

Thank you so much for putting effort into this PR, and my apologies for the late review. I would like to close this for now as it conflicts a lot with the current master. And to be honest, we are not likely to add support for vertical alignment without rewriting the current implementation first, as it is a real mess.

Again, thank you for your work. Sorry that I couldn't review this in a timely manner.

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