Skip to content

Fix #91 by removing the shortcuts from the fitting predicates#97

Closed
sjakobi wants to merge 2 commits intohaskell-prettyprinter:masterfrom
sjakobi:91-properly-2
Closed

Fix #91 by removing the shortcuts from the fitting predicates#97
sjakobi wants to merge 2 commits intohaskell-prettyprinter:masterfrom
sjakobi:91-properly-2

Conversation

@sjakobi
Copy link
Copy Markdown
Collaborator

@sjakobi sjakobi commented Nov 8, 2019

This version passes the fusion property tests using the Unbounded layout.

fits pw m _ (SLine i x)
| m < i, AvailablePerLine cpl _ <- pw = fits pw m (cpl - i) x
| m < i, AvailablePerLine cpl _ <- pw = fits pw m (Just (cpl - i)) x
| otherwise = True
Copy link
Copy Markdown
Collaborator Author

@sjakobi sjakobi Nov 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I don't understand is why it doesn't seem to be necessary to continue checking for the absence of SFail when using the Unbounded layout – at least the property tests don't seem to mind (and neither does dhall!)

@sjakobi
Copy link
Copy Markdown
Collaborator Author

sjakobi commented Nov 8, 2019

The advantage of this solution is that no types need to change. However IMO FittingPredicates are overkill for the Unbounded layout, and I suspect that performance for bounded layouts will be affected too.

IMO having a separate layoutUnbounded function as an alternative to layoutSmart and layoutPretty would still be the more principled approach (see #92 (comment)).

With that approach, FittingPredicate would

  • …lose the Maybe on the maxWidth paramater
  • in layoutWadlerLeijen the FittingPredicate parameter would be replaced with a selectNicer function, that would be very simple in the case of layoutUnbounded, and could be constructed from layout{Pretty,Smart}'s FittingPredicates
  • Unbounded should be removed as a PageWidth constructor, but could stay if we want to accept some internal clunkiness for compatibility reasons

IMHO Unbounded should really die though: It negates the essence of what layoutPretty and layoutSmart are.

@sjakobi
Copy link
Copy Markdown
Collaborator Author

sjakobi commented Nov 8, 2019

  • Unbounded should be removed as a PageWidth constructor, but could stay if we want to accept some internal clunkiness for compatibility reasons

Ah, I had missed some uses of PageWidth, for example in the WithPageWidth Doc constructor, where layoutCompact relies on Unbounded

@sjakobi sjakobi mentioned this pull request Nov 18, 2019
@sjakobi
Copy link
Copy Markdown
Collaborator Author

sjakobi commented Nov 18, 2019

Closing in favour of #98.

@sjakobi sjakobi closed this Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant