-
Notifications
You must be signed in to change notification settings - Fork 42
Rework group and remove fail #126
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
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 | ||
|
||
| = 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.
Outdated
Show resolved
Hide resolved
|
||
| (counterexample ("Grouped (Simple) " ++ (show . diag) (groupedSimple)) | ||
|
||
| (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
flatteninstead ofgrouphere? 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
changesOnFlatteningis 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
flattenandgroupreturnDocs. Andgroupcan 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 usinggrouptocolumn (\_ -> 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.