-
Notifications
You must be signed in to change notification settings - Fork 57
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
erlfmt_format: dont group pipes so they all break together #84
Conversation
'|' -> | ||
with_next_break_fits(IsNextBreakFits, RightD, fun (R) -> | ||
RightOpD = nest(break(concat(<<" ">>, OpD), R), Indent, break), | ||
concat(LeftD, concat(maybe_force_breaks(HasBreak), RightOpD)) |
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 not sure this would behave correctly with mixed operators. For example something like 1 + 2 | 3 + 4
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 to be a parse error "1 + 2 | 3 + 4\n"
and "Foo = 1 + 2 | 3 + 4\n"
I didn't know you could write that, do you have a parsable example?
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 sorry. This should be a complete example that parses fine today:
-type foo() :: 1 + 2 | 3 + 4.
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.
These both work
?assertSame("-type foo() :: 1 + 2 | 3 + 4.\n"),
?assertSame(
"-type foo() ::\n"
" 1 + 2 |\n"
" 3 + 4.\n"
).
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 could add this one as a test
?assertSame(
"-type foo() ::\n"
" 1 + 2 |\n"
" 3 + 4.\n"
).
But maybe you have a different test in mind?
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.
Looks good. It seems the thing I was worried about works correctly. Great job!
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 got Lucky :)
Fixes #53