Skip to content
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

Implement DropAction #58

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Implement DropAction #58

wants to merge 2 commits into from

Conversation

Xitian9
Copy link
Collaborator

@Xitian9 Xitian9 commented Jul 28, 2023

This separates the measurement phase from the building phase.

The tests currently fail, but I'm putting this up as draft to solicit feedback on whether this was what you had in mind. The reasons test fail is that buildCell $ dropLeft n s is no longer guaranteed to have the expected width, since it will no longer add padding to get up to the expected width. Correcting this is either a matter of changing the trimming logic elsewhere, or reimplementing the padding logic for now.

@Xitian9
Copy link
Collaborator Author

Xitian9 commented Jul 29, 2023

There's a question of whether we should require a Monoid instance for all DropActions. This is necessary in order to define a Cell instance for Formatted, but doesn't strictly have to be part of the class definition.

Option 1: class (Monoid (DropAction a)) => Cell a

Advantages

  • Conceptually it makes sense that we should be able to combine DropActions.
  • It slightly simplifies the instances instance Cell a => Cell (Maybe a) and instance (Cell a, Cell b) => Cell (Either a b). The fact that we can use mempty avoids having to introduce a couple of Maybes.
  • This allows the instance Cell a => Cell (Formatted a) without any extra hassle.

Disadvantages

  • Not needed for most simple instances.
  • Implementations need to make sure there are Monoid instances for all DropActions.
  • Requires FlexibleContexts in Cell.hs.

Option 2: instance (Monoid (DropAction a), Cell a) => Cell (Formatted a)

Alternatively, we could leave it out of the class definition and just add it in to the instance declaration instance (Monoid (DropAction a), Cell a) => Cell (Formatted a).

Advantages

  • Don't have to implement Monoid instances for DropActions if you don't want to.

Disadvantages

  • Requires FlexibleContexts and UndecidableInstances in Formatted.hs.
  • If you don't define Monoid instances, then you can't use Formatted.
  • Slightly less clean implementations of instance Cell a => Cell (Maybe a) and instance (Cell a, Cell b) => Cell (Either a b).

I think option 1 has the advantage of being conceptually cleaner and with cleaner code, at the cost of a tiny bit of boilerplate for instance definers (which is probably a good idea to do anyway). The fact that option 2 requires UndecidableInstances is a big point against it in my opinion, and I'd strongly recommend the former.

@Xitian9 Xitian9 force-pushed the dropUnits branch 7 times, most recently from b7b5c9b to 1a39359 Compare July 31, 2023 12:48
This separates the measurement phase from the building phase.
@Xitian9 Xitian9 marked this pull request as ready for review August 20, 2023 01:07
@Xitian9
Copy link
Collaborator Author

Xitian9 commented Sep 11, 2023

I'll have some free time over the next couple of weeks, if you wanted to make suggestions.

@muesli4
Copy link
Owner

muesli4 commented Nov 17, 2023

I'm sorry to leave you hanging again. I know exactly what it feels like from your perspective and it is unfair. Basically any change that takes me more than an hour to review is difficult. I can't make any promises there.

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.

2 participants