-
Notifications
You must be signed in to change notification settings - Fork 36
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
Rework group and remove fail #126
base: master
Are you sure you want to change the base?
Changes from 3 commits
99553dc
714557d
1f89ce8
8811af2
ec330bc
6e47410
fc9d54f
a588c97
4a1a4c7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -514,16 +514,106 @@ softline' = group line' | |
hardline :: Doc ann | ||
hardline = Line | ||
|
||
group :: Doc ann -> Doc ann | ||
group = \doc -> case doc of | ||
FlatAlt x y -> case changesOnFlattening y of | ||
HasLine -> x | ||
NoChange -> Union y x | ||
Flat y' -> Union y' x | ||
|
||
x@(Cat a b) -> case (changesOnFlattening a, changesOnFlattening b) of | ||
(HasLine , _ ) -> x | ||
(_ , HasLine ) -> x | ||
(NoChange , NoChange) -> x | ||
(NoChange , Flat b' ) -> Cat a (Union b' b) | ||
(Flat a' , NoChange) -> Cat (Union a' a) b | ||
(Flat a' , Flat b' ) -> Union (Cat a' b') (Cat a b) | ||
|
||
Annotated ann x -> Annotated ann (group x) | ||
Nest i x -> Nest i (group x) | ||
|
||
Column f -> Column (group . f) | ||
Nesting f -> Nesting (group . f) | ||
WithPageWidth f -> WithPageWidth (group . f) | ||
|
||
x@Union{} -> x | ||
x@Char{} -> x | ||
x@Text{} -> x | ||
x@Line -> x | ||
x@Empty -> x | ||
-- Should never happen on a valid document | ||
x@Fail -> x | ||
|
||
changesOnFlattening :: Doc ann -> FlatteningResult (Doc ann) | ||
changesOnFlattening = \doc -> case doc of | ||
FlatAlt _ y -> case changesOnFlattening y of | ||
HasLine -> HasLine | ||
NoChange -> Flat y | ||
Flat y' -> Flat y' | ||
|
||
Union x _ -> Flat x | ||
|
||
Cat a b -> case (changesOnFlattening a, changesOnFlattening b) of | ||
(HasLine , _ ) -> HasLine | ||
(_ , HasLine ) -> HasLine | ||
(NoChange , NoChange) -> NoChange | ||
(Flat a' , NoChange) -> Flat (Cat a' b) | ||
(NoChange , Flat b' ) -> Flat (Cat a b') | ||
(Flat a' , Flat b' ) -> Flat (Cat a' b') | ||
|
||
Annotated ann x -> Annotated ann <$> changesOnFlattening x | ||
Nest i x -> Nest i <$> changesOnFlattening x | ||
|
||
Column f -> Flat (Column (flatten . f)) | ||
Nesting f -> Flat (Nesting (flatten . f)) | ||
WithPageWidth f -> Flat (WithPageWidth (flatten . f)) | ||
|
||
Line -> HasLine | ||
|
||
-- Should actually be impossible here. HasLine has the same effect tho | ||
Fail -> HasLine | ||
|
||
Text{} -> NoChange | ||
Char{} -> NoChange | ||
Empty -> NoChange | ||
where | ||
flatten :: Doc ann -> Doc ann | ||
flatten = \doc -> case doc of | ||
FlatAlt _ y -> flatten y | ||
Cat x y -> Cat (flatten x) (flatten y) | ||
Nest i x -> Nest i (flatten x) | ||
Line -> Fail | ||
Union x _ -> flatten x | ||
Column f -> Column (flatten . f) | ||
WithPageWidth f -> WithPageWidth (flatten . f) | ||
Nesting f -> Nesting (flatten . f) | ||
Annotated ann x -> Annotated ann (flatten x) | ||
|
||
x@Fail -> x | ||
x@Empty -> x | ||
x@Char{} -> x | ||
x@Text{} -> x | ||
|
||
data FlatteningResult a | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this type have different semantics than If you don't like the constructor names on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I just added names this way to not conflict with the others. No technically you could the other type, just wanted to make a clear distinction, if this ends up performing much better than master and you want to merge it, I can change that 👍 |
||
= HasLine | ||
| NoChange | ||
| Flat a | ||
|
||
instance Functor FlatteningResult where | ||
fmap _ HasLine = HasLine | ||
fmap _ NoChange = NoChange | ||
fmap f (Flat a) = Flat (f a) | ||
|
||
-- | @('group' x)@ tries laying out @x@ into a single line by removing the | ||
-- contained line breaks; if this does not fit the page, @x@ is laid out without | ||
-- any changes. The 'group' function is key to layouts that adapt to available | ||
-- space nicely. | ||
-- | ||
-- See 'vcat', 'line', or 'flatAlt' for examples that are related, or make good | ||
-- use of it. | ||
group :: Doc ann -> Doc ann | ||
simpleGroup :: Doc ann -> Doc ann | ||
-- See note [Group: special flattening] | ||
group x = case changesUponFlattening x of | ||
simpleGroup x = case changesUponFlattening x of | ||
Flattened x' -> Union x' x | ||
AlreadyFlat -> x | ||
NeverFlat -> x | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -90,6 +90,9 @@ tests = testGroup "Tests" | |
, testCase "Line within align" regressionUnboundedGroupedLineWithinAlign | ||
] | ||
] | ||
, testGroup "Group" [ | ||
testProperty "simpleGroup == group" groupLayoutEqualsSimpleGroupLayout | ||
] | ||
] | ||
|
||
fusionDoesNotChangeRendering :: FusionDepth -> Property | ||
|
@@ -111,6 +114,17 @@ fusionDoesNotChangeRendering depth | |
, "Fused:" | ||
, indent 4 (pretty renderedFused) ] | ||
|
||
groupLayoutEqualsSimpleGroupLayout :: Property | ||
groupLayoutEqualsSimpleGroupLayout = forAllShow (arbitrary :: Gen (Doc Int)) (show . diag) (\doc -> | ||
forAll arbitrary (\layouter -> | ||
let grouped = group doc | ||
groupedSimple = simpleGroup doc | ||
groupedLayedOut = layout layouter grouped | ||
groupedSimpleLayedOut = layout layouter groupedSimple | ||
in counterexample ("Grouped: " ++ (show . diag) (grouped)) | ||
sjakobi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
(counterexample ("Grouped (Simple) " ++ (show . diag) (groupedSimple)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This nested There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ^^ Agree, just took a shortcut tbh ;)
sjakobi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
(groupedLayedOut === groupedSimpleLayedOut)))) | ||
|
||
instance Arbitrary ann => Arbitrary (Doc ann) where | ||
arbitrary = document | ||
shrink = genericShrink -- Possibly not a good idea, may break invariants | ||
|
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.
So why do we need to use
flatten
instead ofgroup
here? I think you tried to explain that before but I haven't grokked it yet. ;)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.
Because
changesOnFlattening
is expected to return a value that tells the caller if the doc changed (either by flattening, by encountering a Line or not changing). And you cannot get that information out of a function :/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.
Hm, I don't understand what you're saying here. Both
flatten
andgroup
returnDoc
s. Andgroup
can actually add information by wrapping things inUnion
.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.
Thats what you are thinking about! Yes definitly that is a good idea, I'll test 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.
This fails quickcheck with a huge counterexample again and my own tests with a small one. Now to figure out whats wrong...
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.
Counterexample:
column (\_ -> hardline)
. This flattens tocolumn (\_ -> Fail)
but when usinggroup
tocolumn (\_ -> Line)
.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.
Ah, that's very interesting! Would that explain the test failures I got for the first version of this PR?
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, or at least the problem was very similar iirc.