bal: Show commodity symbol as a subaccount / in a column #1626
Conversation
|
This looks like an excellent start! I tried to understand the new code just by reading the names and haddocks, and had trouble. Perhaps we can find ways to make some of those clearer. The terminal output shows (fake) subaccounts, while the CSV output shows a separate commodity column. Should we have them both do the same thing ? Which is better ? Will the fake accounts be confusing/frustrating, eg will people try to filter by those accounts or view their register ? If the subaccounts are a good idea, would we want to implement them more deeply so they are visible in all reports ? |
|
I think a good approach would be to extend I'm personally in favour of a commodities column instead of fake subaccounts, but if others prefer subaccounts I wouldn't object. |
|
A column seems simpler and a more limited scope than an account name. However it doesn't work for tree-mode balance reports, am I right ? |
|
I think we have big problems for tree-mode balance reports with any method. For example, if we have |
|
We have discussed three places to show commodity symbols.
If we will ultimately want all three of these, we should provide a three-way option, like It seems to me the simplest lowest-impact way to address #1559 is to implement the column mode for both terminal and CSV output. It should give an error when used with unsupported output formats (json...) or incompatible modes (tree). It would be ok to start with this and change our minds later I guess. Thoughts ? |
|
I agree that tree mode would make this 'fake subaccount' view quite confusing. If we adopted the syntax of (9), i.e commodity in brackets after, that could resolve that issue. However, I think that having a consistent column for csv and terminal output is probably the least surprising behavior. I'll rework this to take some of Xitian9's suggestions and the commodity column in both. |
|
Subaccounts should work just fine in tree mode, but let's do the simple thing for now.
|
|
@simonmichael: I think we agree on the course of action to take, but I'm actually not able to picture how subaccounts would work in tree mode. Could you do a quick mockup of how this should appear in tree mode? Assume that we're calling it with |
|
In the case of tree mode, should the account name be repeated? e.g vs Also, in the normal vs |
|
For the multi-column report, I think the second is more consistent with how the normal balance report handles things, and saves a lot of clutter. For the question of aligning columns I don't have a strong opinion, but maybe we should leave this question aside as a minor stylistic tweak until we've sorted the primary behaviour. I think your tree example is an excellent demonstration of the fact that we probably shouldn't worry too much about how it looks in tree mode. It will be clunky and verbose in any case, and flat mode is the primary use case for commodity columns. |
|
Sorry, can't look in detail right now, perhaps I'm missing something about the subaccount scenario. But to be clear I'm suggesting we ignore it for now, just support column display, and error if it's combined with tree mode.
|
|
I've run the branch locally for a few sample reports, and I have to say it's looking great so far! Very clean and concise. I'll give more in-depth feedback when I've had a chance to look in the code in detail, but the results are great. |
|
Is it convenient to squash some of your commits so that changes which are later overwritten disappear? That would make review a bit easier. |
|
We seem to have lost amount elision in ordinary single-line multi-column balance reports. In particular test/balance/multicommodity.test is now failing in tests 1 and 3. |
|
Thanks for taking a look. I've fixed the regression and consolidated the git history as well. Do we care to label everything as |
|
Do we care to label everything as commodity-column now?
Yes --commodity-column sounds decent at the moment.
|
|
Quick test notes, if not out of date:
|
What do you mean by this?
Done, thanks! |
|
It should affect both csv and terminal output. When I tested I saw no change in terminal output.
|
|
It was definitely affecting terminal output on the version I saw. |
|
What am I doing wrong ? |
I don't particularly mind either way we handle it. In case we do want to render the single-period commodity-column differently, would you prefer
For tree mode, commodity-column still does the 'right thing' (see mockups #1626 (comment)). I think the main issue is that it is verbose. |
| renderTableB topts fr fc f = renderTableB' topts (fmap fc) (\(rh, as) -> (fr rh, fmap f as)) | ||
|
|
||
| -- | A version of renderTable that operates on rows | ||
| renderTableB' :: TableOpts -- ^ Options controlling Table rendering |
There was a problem hiding this comment.
I think we should use a more understandable name here, something like renderTableByRowsB'.
There was a problem hiding this comment.
And perhaps a more detailed haddock.
A version of renderTableB which uses a different rendering function for each row.
|
I've taken a look at the remaining code, and it looks great! It's a bit disappointing that budget reports require so much extra code to handle, but they are a bit clunky in any case. I'm sure there's a more principled solution to budget reports in general which will require less cludgy code for many things. If we feel that a separate commodity column should also be put in for single-column balance reports, I think your proposal looks good to me. |
Agreed. I tried to get something working cell-wise (by having
Sounds good, I'll take a look tomorrow. Thanks! |
Yes, I tried to do that at some point too, and that was exactly the problem I ran into. The ideal solution would be to allow headers to have a column width argument, but this would require re-implementing even more of the |
|
Thanks for all the work on this. I'm not following closely just now but in case it's useful to know, I see balance's --format as a bit of a niche feature - I guess it's useful occasionally but it's not something I use personally. Like the non periodic balance report itself, it works well enough that it's hard to justify dropping it. But it's always worth keeping that option in mind if this is costing progress elsewhere.
|
|
For ease of implementation, the single-period balance report in |
|
@Xitian9 @simonmichael What are your thoughts on the PR at this point? Would it be helpful if I rebased/squashed commits again? |
|
Thanks, I was prioritising other things, I'll check this again soon. |
|
It's overall looking very good. I'll be very happy to see this merged soon. I'll do a proper code review later, but here are some quick suggestions:
|
This adds the `--commodity-column` option that displays each commodity on a separate line and the commodities themselves as a separate column. The initial design considerations are at simonmichael.hledger.issues.1559 The single-period balance report with `--commodity-column` does not interoperate with custom formats.
Extension of commodity-column to budget reporting.
Add documentation and sample output for `--commodity-column` behavior and functional tests e.g single-period balance, yearly balance, and yearly budget
Makes them consistent with the remaining cells and fixes awkward alignment issue in commodity-column mode where we don't display anything
Previously would not actually display anything since the Cell's WideBuilders are single-element list. Just dispatch to showMixedAmountB to do the right thing.
99e09c5 to
aefdaaf
Compare
|
Looks good to me! I have no particular code comments. |
|
Thanks for the review. Looking good @la-wu! I have some minor comments about docs, if you don't mind I'm going to merge and just do them. I think this will be a valuable feature, unlocking charting for one thing. Thank you! |
- promote the heading one level - periodic CSV reports are supported too - slight edits to manual and flag description
|
@la-wu : in the doc, the -T in the CSV example looks redundant. More importantly I couldn't reproduce your example, trying the below; what am I missing ? |
|
Ah, found it: -T should be -3. Fixed. |
|
All the examples were based on |
and make examples reproducible, why not.
This is a potential implementation of proposals (6) and (10) from #1559. These commits handle the
balancefamily of commands but should be extendable tocompound-balancefamily as well.