Remove emptyCell (#45)

[muesli4]

Removes emptyCell and implements the empty cell for table headers on a lower level.

Testing done

The tests pass and the executable works as expected.

[Xitian9]

I quite like this: it's much cleaner than emptyCell.

It still leaves open the problem of colsAsRowsAll and colsAsRows. As I see they still use emptyCell. We could always go back to requiring a Monoid constraint and using mempty, but that's still requiring a lawful Semigroup instance which is completely unneeded.

Another option would be to require Default instead and use def, but not everything has default instances defined.

The simplest solution is to just supply the filler to colsAsRows directly: colsAsRows :: a -> [Position V] -> [Col a] -> [Row a].

[Xitian9]

There's also the ‘everybody gets what they want’ solution: leave colsAsRows(All)? as they were before, with a Monoid constraint, and provide a second set of functions colsAsRows(All)?' which take the filler as an argument.

[Xitian9]

I think you also need to replace emptyCell with spacesB (widthCMI cMI) on line 429 of Table.hs

[muesli4]

I think you also need to replace emptyCell with spacesB (widthCMI cMI) on line 429 of Table.hs

Yes, I wonder how that compiled the first time.

What I noticed is that we could technically provide empty cells without requiring anything from the user. Empty cells could be represented as Nothing. Then there are several options:

1. Functions that generate empty cells always generate RowGroup (Maybe a).
   • Extra burden for the user to match types.
2. Functions that generate empty cells use a different constructor in RowGroup a.
   • Adapt functions that use RowGroup slightly.
   • Implementing Cell a => Cell (Maybe a) could make this very easy.
3. RowGroup a uses [[Maybe a]] internally (i.e. every cell is optional).
   • Extra overhead every time.
   • Defining an instance of Cell a => Cell (Maybe a) is easy. Alternatively: Provide specialized functions.
   • Overkill, if the Cell instance is also a Monoid.

For me it seems option number 2 is the most attractive for the user. And making sure our code is generic enough probably also does not hurt.

[muesli4]

I implemented it as described. Some internals related to RowGroup have been moved. I think it is useful because it clearly shows the interface that is required to work with row groups.

Let me know what you think. Some of the names are a bit lacking (e.g., OptionalRowGroup indicates that the whole row group may be missing).

Testing done

Tests pass, although I am afraid there is probably not enough coverage. Maybe I will do this later.

> putStrLn $ tableString [def, def, def] asciiRoundS (titlesH ["row 1", "row 2", "row 3"]) (titlesH ["col 1", "col2"]) [rowG ["a", "b"], rowG ["c", "d"], optionalRowsG [[Just "d", Nothing]]]
.-------.-------.------.
|       | col 1 | col2 |
+-------+-------+------+
| row 1 | a     | b    |
+-------+-------+------+
| row 2 | c     | d    |
+-------+-------+------+
| row 3 | d     |      |
'-------'-------'------'

I did not export optionalRowsG but for testing it is nice.

[Xitian9]

Overall it looks very good.

I have no strong opinions on naming (you may have noticed that my naming skills are not great). Maybe OptionalRowGroup could be GappedRowGroup?

The test you recommend would be a good addition.

[muesli4]

I updated the related issues from https://github.com/muesli4/table-layout/pull/48 here.

[Xitian9]

I've been ignoring this since these changes are included in #48. They look good to me, and I have no further comment.