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

empty table cells in multi-period balance reports [since 1.20] #1526

Closed
lestephane opened this issue Apr 11, 2021 · 19 comments
Closed

empty table cells in multi-period balance reports [since 1.20] #1526

lestephane opened this issue Apr 11, 2021 · 19 comments
Labels
A-BUG Something wrong, confusing or sub-standard in the software, docs, or user experience. balance regression A backwards step, indicating a weakness in our QA. We don't like these.

Comments

@lestephane
Copy link

$ hledger bal -f tmp.journal --yearly Income:Gains
Balance changes in 2018-01-01..2021-12-31:

              || 2018  2019  2020  2021
==============++========================
 Income:Gains ||    0                 0
--------------++------------------------
              ||    0                 0
$ hledger bal -f tmp.journal -p 2019 Income:Gains
-93.56707511000000000000000000 EUR  Income:Gains
--------------------
-93.56707511000000000000000000 EUR
$ hledger bal -f tmp.journal -p 2020 Income:Gains
-1094.29560548000000000000000000 EUR  Income:Gains
--------------------
-1094.29560548000000000000000000 EUR

I don't understand what is going on here. The --yearly report has always worked for me, and that stopped recently.
Now I cannot decide whether my understanding is wrong, or hledger has regressed. In any case, it is surprising.

@lestephane lestephane added the A-BUG Something wrong, confusing or sub-standard in the software, docs, or user experience. label Apr 11, 2021
@lestephane
Copy link
Author

Forgot to mention that it's on 1.21.

@simonmichael simonmichael added balance regression A backwards step, indicating a weakness in our QA. We don't like these. labels Apr 11, 2021
@simonmichael
Copy link
Owner

Thanks for the report. That's unfortunate! Would you be able to boil it down to an example journal ?

@lestephane
Copy link
Author

Thanks for the report. That's unfortunate! Would you be able to boil it down to an example journal ?

I'll try, but In the event that I'm unsuccessful, will you have a way to receive the tmp.journal securely?
gpg, onionshare, keybase, whatever...

@lestephane
Copy link
Author

I use a lot of piped hledger print -f - commands, and commodity directives don't seem to be preserved in the output of those print commands. So tmp.journal does not have a commodity directive. Adding it back makes everything magically work.

(echo "commodity 1000.00 EUR"; cat tmp.journal) | hledger bal -f - --yearly Income:Gains
Balance changes in 2018-01-01..2021-12-31:

              || 2018        2019          2020  2021
==============++======================================
 Income:Gains ||    0  -93.57 EUR  -1094.30 EUR     0
--------------++--------------------------------------
              ||    0  -93.57 EUR  -1094.30 EUR     0

So the failure mode seems formatting related.

@lestephane
Copy link
Author

There's a rather large count of digits after the decimal point for most amounts in the tmp.journal, since it came from unrealized gains calculations computed in beancount.

$ grep -oE "[0-9]+\.[0-9]+ EUR" tmp.journal | cut -d ' ' -f 1 | cut -d '.' -f 2 | awk '{print(length)}' | sort -rn  | uniq -c
      2 29
      5 28
     46 27
      4 26
      2 25
    273 24
      6 23
    547 10
   2789 8
    194 4

@simonmichael
Copy link
Owner

Sure, if you have a matrix client set up, encrypted direct chat to me (simonmic:matrix.org) could be an easy way.

@Xitian9
Copy link
Collaborator

Xitian9 commented Apr 12, 2021

I'm curious: do you still get this issue in master, or with the branch in #1509? I've put some effort into refactoring the code so this sort of bug (easy to accidentally do in the past) is less likely/impossible to do now.

@simonmichael simonmichael added the needs-release This should probably be shipped in a release soon. label Apr 12, 2021
@lestephane
Copy link
Author

I'm curious: do you still get this issue in master, or with the branch in #1509? I've put some effort into refactoring the code so this sort of bug (easy to accidentally do in the past) is less likely/impossible to do now.

@Xitian9 I will check today, and let you know

@lestephane
Copy link
Author

@Xitian9 tried your mixedamt branch. Same problem. I'm going to try to shrink the journal, and if not, i'll need to setup a matrix client to send @simonmichael the journal.

@lestephane
Copy link
Author

Here is the smallest journal that exhibits the problem on my end:

cat <<EOF | ~/.local/bin/hledger-1.21 bal -f - --yearly Income:Gains

2020-12-30 * Sold BTC to EUR
  Income:Gains                                                      -0.08724382 EUR
  Assets:XBT                                                       -0.00000437 XBT @ 2919.034429281334766023231553 EUR
  Assets:EUR                                                         0.10000000 EUR
  Equity:Rounding  ; rounding from beancount: 0.0000000004559594329275215219 EUR

2021-04-11 U Unrealized loss for 14.23000000 units of GBP (price: 1.1526 EUR as of 2021-04-09, average cost: 1.1680 EUR)
  Assets:GBP:Unrealized                                            -0.21871744220000000000000000 EUR
  Income:GBP:Unrealized                                            0.21871744220000000000000000 EUR
  Equity:Rounding  ; rounding for hledger

EOF

Results in

Balance changes in 2020-01-01..2021-12-31:

              || 2020  2021
==============++============
 Income:Gains ||          0
--------------++------------
              ||          0

@simonmichael
Copy link
Owner

Thanks!

@Xitian9, I think showAmountsOneLineB shows nothing if the rendered amount would exceed the hard-coded max width of 32 in Hledger.Cli.Commands.Balance.balanceOpts. (In this example, writing 25 instead of 26 decimal places makes it work.) It should show an elided amount instead, I guess.

@simonmichael
Copy link
Owner

simonmichael commented Apr 13, 2021

Or, allow cells to grow as wide as needed ?

[Start of a test:]

<
2021-01-01
    (a)      -0.12345678901234567890123456 EUR

$ hledger -f- bal -YN
Balance changes in 2021:

   ||                              2021 
===++===================================
 a || -0.12345678901234567890123456 EUR 

@simonmichael simonmichael changed the title empty cells (not even value 0) where there should be a value present, in a --yearly bal report empty cells (not even value 0) where there should be a value present, in a --yearly bal report [since 1.20] Apr 13, 2021
@simonmichael simonmichael changed the title empty cells (not even value 0) where there should be a value present, in a --yearly bal report [since 1.20] empty table cells in multi-period balance reports [since 1.20] Apr 13, 2021
@Xitian9
Copy link
Collaborator

Xitian9 commented Apr 14, 2021

It's not clear how to elide a single amount. Here are some options:

  1. Elide from the left
    • Simple
    • May elide commodity symbol
    • Elides most significant digits
  2. Elide from the right
    • Simple
    • May elide commodity symbol
    • If there is no visible decimal place, cannot determine the order of magnitude of the number
  3. Elide from the left, but don't elide commodity symbol
    • Similar to (1), but doesn't elide commodity symbol.
    • Still runs into problems with long commodity symbols.
  4. Elide from the right, but don't elide commodity symbol
    • Similar to (2), but doesn't elide commodity symbol.
    • Still runs into problems with long commodity symbols.
  5. Use scientific notation for elided numbers
    • Number is clear
    • Still runs into problems with long commodity symbols

Or, as you say, we can just ignore the maximum width if it would cause nothing to display. This might mess up alignment unless we put some extra effort into that.

@simonmichael
Copy link
Owner

simonmichael commented Apr 14, 2021 via email

@Xitian9
Copy link
Collaborator

Xitian9 commented Apr 14, 2021

We currently do eliding by displaying only some of the amounts in a MixedAmount; we don't ever elide the string representing a single amount.

In 1.19, exactly two quantities would be displayed, with no regard for their maximum width.

@Xitian9
Copy link
Collaborator

Xitian9 commented Apr 14, 2021

So perhaps the best option is to elide according to the previous rules, but make sure we always display at least one commodity along with an elision string, even if that would put us over the hard-coded width. That width is pretty arbitrary in any case.

Xitian9 added a commit to Xitian9/hledger that referenced this issue Apr 14, 2021
…t one amount, even if that would exceed the requested maximum width. Addresses simonmichael#1526.
@simonmichael
Copy link
Owner

Sounds pretty good..

Xitian9 added a commit to Xitian9/hledger that referenced this issue Apr 14, 2021
…t one amount, even if that would exceed the requested maximum width. Addresses simonmichael#1526.
simonmichael pushed a commit that referenced this issue Apr 14, 2021
…t one amount, even if that would exceed the requested maximum width. Addresses #1526.
@simonmichael
Copy link
Owner

Fixed in master by #1530.

@lestephane
Copy link
Author

I can confirm that it is fixed in master 0f4e462. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-BUG Something wrong, confusing or sub-standard in the software, docs, or user experience. balance regression A backwards step, indicating a weakness in our QA. We don't like these.
Projects
None yet
Development

No branches or pull requests

3 participants