fix: bal: fix commodity-column interaction with transpose and empty commodities

[0xlwu]

I was introducing --commodity-column into my personal reports and realized that the textual output didn't correctly handle --transpose when both flags are set. While fixing that, I also realized that empty commodities were being silently filtered out (e.g https://github.com/simonmichael/hledger/blob/master/hledger/Hledger/Cli/Commands/Balance.hs#L717). This wasn't a regression per-se but obviously undesirable behavior.

There is some overlap with #1649 since handling transposition unfortunately involved some reworking of the compound call-site.

Perhaps the suite of flag interactions should be more formally considered?

[Xitian9]

I haven't done a full read-through of the code here, but I think we shouldn't worry about conflicts between PRs for merging this: I can easily rebase.

My bigger thought is that we seem to be jumping through a lot of hoops to work around the limitations of the table rendering code. If I understand correctly, the problem here is that we basically have two columns worth of row headers (account name, and commodity), while the rendering code can only handle one. So we are ad-doc throwing commodity columns in using purpose-designed functions.

Would it be reasonable to solve this problem with a transpose option in the rendering code? Perhaps to add transposeTable to TableOpts, which would flip the roles of the rows and columns in the rendering function?

[Xitian9]

Thinking about it: you have a working implementation here. Let's work on merging this with the basic method used here. If we come up with a simpler way later, we can do the refactoring then and make sure it still passes all these tests.

[0xlwu]

My bigger thought is that we seem to be jumping through a lot of hoops to work around the limitations of the table rendering code. If I understand correctly, the problem here is that we basically have two columns worth of row headers (account name, and commodity), while the rendering code can only handle one. So we are ad-doc throwing commodity columns in using purpose-designed functions.

Would it be reasonable to solve this problem with a transpose option in the rendering code? Perhaps to add transposeTable to TableOpts, which would flip the roles of the rows and columns in the rendering function?

That is certainly one way to approach it. Are you imagining a situation where renderTable first creates the full table of strings and then does transposition and alignment after (Cell becomes a single WideBuilder)?

Either way we would also need to implement the header-column independence for the budget report that we talked about #1626 (comment) (Which might indicate a compelling use case). Budget was also the most annoying piece of code to change so I'm open to improvements there...

Thinking about it: you have a working implementation here. Let's work on merging this with the basic method used here. If we come up with a simpler way later, we can do the refactoring then and make sure it still passes all these tests.

Sounds good to me. Would it be helpful if I split this into separate PRs for empty-commodity handling fixes and transpose-flag fixes?

[Xitian9]

Sounds good to me. Would it be helpful if I split this into separate PRs for empty-commodity handling fixes and transpose-flag fixes?

That would keep the commit history cleaner, but it's not crucial.

This PR seems to include enabling --commodity-column for compound balance reports. Does this render #1649 completely redundant?

[0xlwu]

This PR seems to include enabling --commodity-column for compound balance reports. Does this render #1649 completely redundant?

I didn't touch the format specifier or any of the aggressive account name elisions so not completely covered. (The CSV and HTML output changes in the compound reports is handled by this and not 1649 though)

[Xitian9]

In that case, I'll mark #1649 as draft and wait until this one is merged before deciding what in there is still useful.

[Xitian9]

This build failure seems to be unrelated to the content of the PR.

[simonmichael]

This build failure seems to be unrelated to the content of the PR.

Pushed a fix that might help.

[simonmichael]

...to master; if this PR gets rebased its CI may work again.

[simonmichael]

Thanks for the followup testing and fixes!