-
Notifications
You must be signed in to change notification settings - Fork 59
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
preserve new lines in guards #115
Conversation
src/erlfmt_format.erl
Outdated
@@ -653,7 +653,7 @@ clause_to_algebra({spec_clause, _Meta, Head, [Body], Guards}) -> | |||
guard_to_algebra(Guards, Separator) -> | |||
GuardsD = lists:map(fun expr_to_algebra/1, Guards), | |||
Doc = fold_doc(fun(GuardD, Acc) -> break(concat(GuardD, Separator), Acc) end, GuardsD), | |||
group(Doc). | |||
group(concat(maybe_force_breaks(has_any_break_between(Guards)), Doc)). |
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 entirely sure we should do this for all guards everywhere. What do you think about limiting it for now only to specs?
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.
Yes I think it would make sense to only do this for spec_clause
I tried this angle first, since doing it for spec_clause only would mean that we need to duplicate the path through expr_to_algebra for gaurd_or and gaurd_and
Line 641 in a3cb176
clause_to_algebra({spec_clause, _Meta, Head, [Body], Guards}) -> |
But I can definitely try that :)
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.
Yes, it definitely increases the complexity a bit in here.
A workaround for that so that we don't need to duplicate the functions could be to add a special metadata to the guard nodes and later consume it in guard_to_algebra
(passing in the additional Meta
argument from expr_to_algebra
).
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.
It ended up being less duplication than I initially thought, so I went with the duplication route.
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! I like the tests checking both a positive and a negative case
Fixes #114