muesli4 / table-layout

Layout data in grids and pretty tables. Provides a lot of tools to get the cell formatting right (positional alignment, alignment on specific characters and limiting of cell width)..
BSD 3-Clause "New" or "Revised" License
38 stars 11 forks source link

Make Cell lazier to allow more instances #44

Closed Xitian9 closed 1 year ago

Xitian9 commented 1 year ago

Previously Cell required being able to drop from the left or right without changing the type of the object. This made it awkward to define instances for which this was not natural.

Cell will no longer drop from the left or right immediately. Instead, drop(Left|Right|Both) will record the amount that has been requested to be dropped, and drop it when buildCell is called.

Changes to the API require that instance declarations for Cell be changed as follows:

1. instance Cell a where dropLeft = f dropRight = g ...

can be changed to

instance Cell a where buildCellView = buildCellViewLRHelper buildCell f g ...

2. instance Cell a where dropBoth = f ...

can be changed to

instance Cell a where buildCellView = buildCellViewBothHelper buildCell f ...

3. instance Cell a where dropLeft = f dropRight = g dropBoth = h ...

can be changed to

instance Cell a where buildCellView = buildCellViewHelper buildCell buildCell buildCell f g h ...

Since dropLeft, dropRight, and dropBoth are no longer class methods of Cell, they may need to be imported explicitly. Code which relies on dropLeft, dropRight, and dropBoth not changing the type of the output may need to be rewritten, possibly by calling buildCell on the result.

Xitian9 commented 1 year ago

Answering your questions from #37 here.

Some questions that should be answered first:

  • Is the context where drop is used only to feed it into buildCell?

Within this library, that is indeed the case. You can see from the fact that, aside from in the test suite, no code needed to be changed and it still passes all tests.

  • Could there be any issues with multi-width characters? Since you implemented the functionality you probably know better.

I think this actually makes multi-width characters easier, since we don't have to add the padding at each step. The proof will be in the pudding.

  • Could this possibly break other use cases of Cell instances? I guess one could simply always use the wrapper as a drop-in replacement of Cell.

It might, but I think we are reasonably safe.

  • Would we mirror functions of Cell that work on the type?

I think they all work as is. Thoughts?

  • How do the Eq and Ord instances behave?

I was thinking that they would be the auto-derived instances: compare the base items, and only if they're equal move on to the left adjustment, and then the right adjustment. That said, this functionality is not used anywhere, so if you object we could leave it out.

  • drop is probably not very useful, as it would introduce nesting, so you would always have to join again.

I think it's not terrible. The following will basically eliminate the need to consider CellView at all:

instance Cell a => Cell (CellView a) where
  buildCellView = buildCellView . join
  buildCell = buildCellView
  ...

The two methods means that, whenever you call buildCell on an arbitrarily nested stack of CellView, it will successively join them all and build, so the stacking will be collapsed automatically.

  • Would it make sense to measure the alignment of an already modified Cell? Previously, the former was used as a step for the latter.

Yes, I agree this step would probably not be used in practice. But it can be implemented sensibly, and buildCell greatly simplifies working with this.

  • We could provide a length function for the views. Other than that I don't really see why we would need something else.

Are you proposing partially implementing the Cell typeclass for CellView, or removing some more functionality from Cell?

Xitian9 commented 1 year ago

The Monad instance of CellView is isomorphic to `Writer (Sum Int, Sum Int), so you can also see that it is lawful that way.

muesli4 commented 1 year ago

Would you mind discussing the issue about emptyCell in #46? Then we may remove the last commit again.

Xitian9 commented 1 year ago

I've removed the last commit: it looks quite redundant considering #46.

muesli4 commented 1 year ago

The pull request has been merged manually.