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

Switch to table-layout for tabular layout #1850

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Xitian9
Copy link
Collaborator

@Xitian9 Xitian9 commented Mar 29, 2022

Fixes #1679. This gets rid of our home-customised Text.Tabular.AsciiWide and Text.WideString modules in favour of the external library table-layout.

At the moment this relies on a few PRs awaiting merge over at table-layout so it's probably not ready to be merged quite yet, but I'm putting it here for comment. I've broken it into several commits so that the changes are more bite-sized, but these changes are not atomic and tests will fail in the middle (mostly due to colouring negative amounts not being reintroduced until showMixedAmountB being updated), though it compiles at each step.

@Xitian9 Xitian9 force-pushed the table-layout branch 3 times, most recently from 38a1ee2 to 5ffa4bb Compare March 29, 2022 23:44
@Xitian9
Copy link
Collaborator Author

Xitian9 commented Mar 30, 2022

This PR would open the door to a number of potential improvements/customisations.

╭───╥───────────────────┬───────────────────┬────────────┰────────────┬────────────┬────────────┬────────────┰────────────┬────────────╮
│   ║     Some text     │   Some numbers    │     X      ┃     Z      │     W      │     A      │     B      ┃    Text    │     Y      │
╞═══╬═══════════════════╪═══════════════════╪════════════╋════════════╧════════════╪════════════╧════════════╋════════════╧════════════╡
│ 1 ║ This is long text ┆    4.20000000     ┆    foo     ┃    blah         bloo    ┆    blop         blog    ┃   Short         baz     │
│   ║       Short       ┆ 200300400500600.2 ┆    bar     ┃   yadda         yoda    ┆   yeeda         york    ┃   Short        wibble   │
├───╫┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┼┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┼┄┄┄┄┄┄┄┄┄┄┄┄╂┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┼┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄╂┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┤
│ 2 ║ This is long text ┆    4.20000000     ┆    foo     ┃  bibbidy      babbidy   ┆    boo          blue    ┃   Short        wobble   │
╰───╨───────────────────┴───────────────────┴────────────┸─────────────────────────┴─────────────────────────┸─────────────────────────╯

@ghost
Copy link

ghost commented Jul 15, 2022

Review on Crocodile

It also means that --layout=bare can function with custom format
strings. The commodity column will be displayed immediately after the
totals column, left aligned. Default width for a balance report with
--layout=bare is harmonised with the ordinary balance report.
of WideBuilder.

Use buildCell to convert this to String, Text, Builder, or other
representable forms.
Text.WideString and Text.Tabular.AsciiWide modules are now redundant and
can be removed. A local definition of Table and concatTables has been
moved to Hledger.Utils.Text.
showMixedAmountOneLineB will now return an ElidableList. This will be
padded or trimmed automagically when rendered with grid or
table-producing functions, or with the pad or trim functions.

The return types of showMixedAmount(|Lines|OneLine)B have changed, but
since the return types are still instances of Cell they can be treated
the same: just use buildCell to render as you will.
@simonmichael
Copy link
Owner

I know you already updated this a bunch of times, but when you get time could you do it once more and I'll review and merge if possible.

The hledger-lib/Text/WideString.hs utils are might be useful down the road, could we to keep them around a bit longer ?

As you saw in chat I would like to tweak table borders, adding consistent separators for totals. Maybe also using the same style for all borders, making life simpler (rather than adding more complex styling - but no harm to have that ability in the toolbox).

How could using table-layout help with decimal aligning ?

@simonmichael
Copy link
Owner

How could using table-layout help with decimal aligning ?

"Align content in a column at a character occurence.", I see, nifty. Might be a bit tricky with our variable decimal marks..

@Xitian9
Copy link
Collaborator Author

Xitian9 commented Oct 25, 2022

"Align content in a column at a character occurence.", I see, nifty. Might be a bit tricky with our variable decimal marks..

It's possible with a custom implementation of Cell. See here: muesli4/table-layout#32

@simonmichael
Copy link
Owner

muesli4/table-layout#43

@simonmichael simonmichael added the needs:other-task To unblock: needs some other issue/task/event, possibly outside our project label Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:other-task To unblock: needs some other issue/task/event, possibly outside our project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replacing/Breaking off Text.Tabular.AsciiWide.
2 participants