Conversation
mrkkrp
left a comment
There was a problem hiding this comment.
Also, for every change like this a changelog entry and a new test(s) are necessary.
src/Ormolu/Printer/Internal.hs
Outdated
| enterLayout, | ||
| vlayout, | ||
| getLayout, | ||
| setAnchor, |
There was a problem hiding this comment.
Try to solve the issue differently without introducing new primitives. We should try to keep our vocabulary of combinators from exploding and try to work within the DSL that we have. Look at my comments in #712 they should suggest how this can be solved and what layout is desirable.
There was a problem hiding this comment.
A new primitive was required to create something like sitcc but just for the first line of each multiline block, like in
foobarbar :: Int -> Bool
foobarbar | x <-
5,
y <-
6 = case x of
5 -> True
_ -> FalseAccording to your last comment on the linked issue, this is no longer what we want to achieve, so my commit doesn't make sense anymore.
There was a problem hiding this comment.
I'm sorry for unclear communication from my side, this alignment feature is not what I had in mind. As a rule, we do not align anything in Ormolu, so y would never have to be aligned with x because if x moved, y would also need to move—this is exactly what we try to avoid in order to help with minimization of diffs.
There was a problem hiding this comment.
No problem, we didn't really discover the case with multiple pattern binds at first. At least, it was a fun exercise to add this functionality in ormolu, and helped me to get a better understanding of how internals work
c34a33d to
63beaaa
Compare
|
Ok, I think I found a way to achieve the requested behavior, and it doesn't seem to introduce changes except in the desired place when applied to the ormolu codebase (via |
|
I think it works as expected now. |
| then blockPlacement placer grhssGRHSs | ||
| else Normal | ||
| Just spn -> case style of | ||
| Function _ | hasGuards && any guardNeedsLineBreak grhssGRHSs -> Normal |
There was a problem hiding this comment.
I am not sure if this was discussed, but is it intended that this change is only applied for the Function style? Example: Even with this PR, the input
\case x | x <-
foo -> case x of
5 -> True
_ -> Falseis formatted to
\case
x | x <-
foo -> case x of
5 -> True
_ -> False(note that there is no linebreak before the |). Simply ignoring style passes all tests locally, and the output would be
\case
x
| x <-
foo -> case x of
5 -> True
_ -> Falsewhich now is consistent with the output for regular function match groups.
There was a problem hiding this comment.
That's a good point indeed, and we didn't discussed it before. However, I don't have the authority to decide whether we should extend it to all styles or not :)
But IMO, it's better to extend it!
I will make a new commit where it is extended to all styles
f8254b5 to
ab78f2a
Compare
Now guards are printed on a new line if at least one guard is multiline or if all guards together occupy more than one line. The body of each guard is also indented one level deeper in that case.
ab78f2a to
7c5aa80
Compare
mrkkrp
left a comment
There was a problem hiding this comment.
Great work! This was not an easy one to figure out. I took the liberty of adjusting this a little bit:
- The new examples that you have added should be without the module headers and the main function, because those parts are completely optional. In fact, they only add visual noise. I removed them.
- There is one interesting corner case that you have skipped: all guards in a single line. I added this example and it showed that we had a bug in our guard printing code! We need to separate guards with breakpoints, not with newlines.
- Please format your code so that it fits in 80 line limit. Try to avoid very long lines.
Close #712.
I don't think this solution is acceptable, but it's still a starting
point to see that it can be made if we really want it.