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

New "-V" flag behavour not reverse compatible when combined with "--end" flag #1083

Closed
trevorld opened this issue Sep 3, 2019 · 14 comments

Comments

@trevorld
Copy link

commented Sep 3, 2019

In particular in hledger the "-V" when combined with the "--end" flag used to only use price directives up to the end date to determine market value but now it also uses price directives after the end flag.

Example:

$ cat error.hledger 

P 2016/01/01 00:00:00 SP                        250 USD
P 2017/01/01 00:00:00 SP                        500 USD

2016-01-01 * Brokerage | Buy Stock
  Assets:Checking  4 SP @ 250 USD
  Equity:Opening  -1000 USD
$ hledger -f error.hledger register -V --end=2016-12-31
2016/01/01 Brokerage | Buy Stock                                                 Assets:Checking                                                           2000 USD      2000 USD
                                                                                 Equity:Opening                                                           -1000 USD      1000 USD
$ hledger -f error.hledger register -V --end=2017-12-31
2016/01/01 Brokerage | Buy Stock                                                 Assets:Checking                                                           2000 USD      2000 USD
                                                                                 Equity:Opening                                                           -1000 USD      1000 USD

The old behaviour used to be equivalent to the new "--value=END-DATE" behaviour:

$ hledger -f error.hledger register --value=2016-12-31
2016/01/01 Brokerage | Buy Stock                                                 Assets:Checking                                                           1000 USD      1000 USD
                                                                                 Equity:Opening                                                           -1000 USD             0

Is this new "-V" combined with "--end=YYYY-MM-DD" behaviour considered a bug that will be fixed? Or do I need to update r-ledger to detect which version of hledger is being used and if the version is new enough use the -value=END-DATE and if older instead use -V --end=END-DATE?

@trevorld trevorld added the A BUG label Sep 3, 2019

@simonmichael

This comment has been minimized.

Copy link
Owner

commented Sep 3, 2019

That's interesting, thanks. Reviewing what you've found, old behaviour first:

$ hledger-1.14 print
2016/01/01 * Brokerage | Buy Stock
    Assets:Checking    4 SP @ 250 USD
    Equity:Opening          -1000 USD

-V shows the SP amount in USD, using the market price on today's date by default. (Ignore the fact that print with -V looks unbalanced, that's expected.):

$ hledger-1.14 print -V
2016/01/01 * Brokerage | Buy Stock
    Assets:Checking        2000 USD
    Equity:Opening        -1000 USD

With a report end date specified, -V uses the market price on that date:

$ hledger-1.14 print -V -e 2016/12/31
2016/01/01 * Brokerage | Buy Stock
    Assets:Checking        1000 USD
    Equity:Opening        -1000 USD

Now the new behaviour - print -V still uses today's market price by default:

$ hledger print -V
2016/01/01 * Brokerage | Buy Stock
    Assets:Checking        2000 USD
    Equity:Opening        -1000 USD

It no longer pays attention to the report end date:

$ hledger print -V -e 2016/12/31
2016/01/01 * Brokerage | Buy Stock
    Assets:Checking        2000 USD
    Equity:Opening        -1000 USD

Unless you set the valuation date explicitly, to "end":

$ hledger print  --value=end -e 2016/12/31
2016/01/01 * Brokerage | Buy Stock
    Assets:Checking        1000 USD
    Equity:Opening        -1000 USD

Is this a bug ? Well, report end date and valuation date are independent concepts; you should be able to see any period's data, valued using the prices on any date. So should a report end date also set the valuation date ? I don't think so.

https://hledger.org/hledger.html#effect-of-value-on-reports is supposed to document how print is affected, and seems a bit unclear.

https://github.com/simonmichael/hledger/blob/master/hledger-lib/Hledger/Reports/EntriesReport.hs#L47 helps determine the default valuation date used by print. In this case I can see it's being used only with --value=end (evaluating to 2016-01-02); it's not used with -V.

https://github.com/simonmichael/hledger/blob/master/hledger-lib/Hledger/Data/Posting.hs#L337 shows why: --value=end executes the AtEnd case. -V executes the second AtDefault case, which uses today's date (same as --value=now).

Is this a bug ? Is there a better default valuation date for -V when valuing postings, affecting print and register ? Still not sure, I'll need a fresh mind.

simonmichael added a commit that referenced this issue Sep 3, 2019
@simonmichael

This comment has been minimized.

Copy link
Owner

commented Sep 3, 2019

@trevorld

This comment has been minimized.

Copy link
Author

commented Sep 3, 2019

Well I reported it as a bug since the release notes claim "There is a new valuation option --value=TYPE[,COMM], with backwards-compatible -B/--cost, -V/--market, -X/--exchange=COMM variants" but there is actually new non-backwards-compatible behaviour that breaks how r-ledger calculates historical net worth in hledger files.

If you want to keep the new behaviour I can first figure out which version of hledger the user has and then use different flags based on whether it is hledger (>= 1.15) or hledger (< 1.15) (which does not have the new --value=DATE option).

I suspect that using the reporting end date as the default valuation date for "--market" is the most useful default market valuation date (which it seems you continue to use as the default for multiperiod reports) instead of --value=now (the new default for single period reports). If users really want the valuation date to differ from the reporting end date couldn't they now set different dates to --value and -end?

@simonmichael

This comment has been minimized.

Copy link
Owner

commented Sep 3, 2019

I had a weaker notion of backwards-compatible in mind there, but you're right. And as you say, "-V == --value=end, always" would seem both more compatible and simpler, and at least as useful, I think. And I think we could do this in a bugfix release quite soon.

@simonmichael

This comment has been minimized.

Copy link
Owner

commented Sep 4, 2019

"-V == --value=end" isn't fully compatible either:

P 2000/01/01 A  2 B
P 2002/01/01 A  3 B

2000/01/01
  (a)      1 A

Old -V uses prices on -e date, otherwise today's date:

$ hledger-1.14 reg -V -e2001; hledger-1.14 reg -V
2000/01/01                      (a)                            2 B           2 B
2000/01/01                      (a)                            3 B           3 B

1.15 single-period -V uses prices on today's date, ignoring -e date:

$ hledger-1.15 reg -V -e2001; hledger-1.15 reg -V
2000/01/01                      (a)                            3 B           3 B
2000/01/01                      (a)                            3 B           3 B

-V equivalent to --value=end uses prices on -e date, otherwise journal end date:

$ hledger-1083 reg -V -e2001; hledger-1083 reg -V
2000/01/01                      (a)                            2 B           2 B
2000/01/01                      (a)                            2 B           2 B
@simonmichael

This comment has been minimized.

Copy link
Owner

commented Sep 4, 2019

I think that last behaviour might be ok from a usability perspective, even though it's a change. How would it affect your usage ?

@simonmichael

This comment has been minimized.

Copy link
Owner

commented Sep 4, 2019

Whether -V (with no report end date specified) should be changed to use the journal end date instead of today as default valuation date.

Pro:

  • When reviewing finances from the babylonian cuneiform tablets, or from last year, -V would show the contemporary value from (end of) that period. Perhaps a more useful default ? (To see value at today's prices, you'd have to say -V -e today, or --value=now.)
  • It would remove the difference between -V's behaviour in single-period and multi-period reports, making it simpler to document and understand. (Currently bal -V shows today's value, while bal -V -Y shows values at each year end.)

Con:

  • It would be different from past hledger.
  • It would be different from Ledger.
@trevorld

This comment has been minimized.

Copy link
Author

commented Sep 4, 2019

When importing in an hledger register (without the user specifying an end-date) r-ledger adds a market value column to the imported register constructed by just using the -V flag. So theoretically it seems the market value column in the imported register r-ledger presents could be affected for a subset of r-ledger's hledger users:

  1. Users with both future transactions and future price directives
  2. Users with price directives before today recorded after their last transaction.

Neither the r-ledger unit tests nor my personal ledgers should be affected since they have neither future price directives nor do they lack any transactions after any price directives and I suspect the majority of users wouldn't be impacted either way but I think (if you don't want to use the reverse-compatible today as the default) then the slightly more useful default if no end date is given would be to use both the price directive dates and the transaction dates in the calculation of the "journal end date" - this would then be equivalent to the current behaviour if there are no future price directives (which are probably rare).

One use case would be someone keeping track of an inventory or investments in a separate ledger that they are holding long-term and while they are keeping the price directives up to date they don't have any new transactions to record but they are still interested in knowing what their current market values are in that separate ledger. Not sure how many such users with that use case exist, if you keep everything (including checking account and credit card) in one ledger and don't have future price directives then you aren't likely to be impacted either way.

@simonmichael

This comment has been minimized.

Copy link
Owner

commented Sep 4, 2019

For the record, price directive dates is something we don't currently consider at all when choosing valuation date. I don't know if Ledger does.

There are are a lot of dates and cases to keep track of. Tricky! And this is reflected a bit in the code right now. So this might take a little longer to bake.

Currently I lean towards just fixing the issue you reported (make -V use -e) and maximising compatibility.

simonmichael added a commit that referenced this issue Sep 5, 2019
valuation: -V/-X respects report end date, code/doc cleanups (#1083)
-V (and -X) now respects a report end date set with -e/-p/date: when
choosing the valuation date, similar to hledger 1.14 and Ledger.

This means that -V/-X aren't exactly like either --value=end or
--value=now. The "Effect of --value on reports" doc has been extended
accordingly, and much of it has been reworded and made more accurate.
@simonmichael

This comment has been minimized.

Copy link
Owner

commented Sep 5, 2019

This is now fixed in master, and these docs should be accurate (for master): https://hledger.org/hledger.html#effect-of-value-on-reports

simonmichael added a commit that referenced this issue Sep 5, 2019
@trevorld

This comment has been minimized.

Copy link
Author

commented Sep 5, 2019

Thanks!

@trevorld trevorld closed this Sep 5, 2019

@simonmichael

This comment has been minimized.

Copy link
Owner

commented Sep 5, 2019

And, released as hledger-1.15.2.

@trevorld

This comment has been minimized.

Copy link
Author

commented Sep 5, 2019

FYI: Just downloaded 1.15.2 and noticed there is a small discrepancy from the old HLedger behaviour (and what Ledger does) but I think the new HLedger behaviour is a more intuitive default than the old behaviour.

Old behaviour New behaviour
Transactions strictly before end date Transactions strictly before end date
Prices before and on end date Prices strictly before end date
$ cat error.hledger 

P 2016/01/01 00:00:00 SP  250 USD
P 2017/01/01 00:00:00 SP  500 USD
P 2017/01/02 00:00:00 SP  750 USD

2016-01-01 * Brokerage | Buy Stock
  Assets:Checking  4 SP @ 250 USD
  Equity:Opening  -1000 USD
2016-12-31 * Brokerage | Dividend
  Assets:Checking  20 USD
  Income:Dividend  -20 USD
2017-01-01 * Brokerage | Dividend
  Assets:Checking  20 USD
  Income:Dividend  -20 USD
$ hledger -f error.hledger register -V --end=2017-01-01
2016/01/01 Brokerage | Buy Stock                    Assets:Checking                              1000 USD      1000 USD
                                                    Equity:Opening                              -1000 USD             0
2016/12/31 Brokerage | Dividend                     Assets:Checking                                20 USD        20 USD
                                                    Income:Dividend                               -20 USD             0
$ ledger -f error.hledger register -V --end=2017-01-01
16-Jan-01 Brokerage | Buy Stock           Assets:Checking                                1000 USD           1000 USD
                                          Equity:Opening                                -1000 USD                  0
16-Dec-31 Brokerage | Dividend            Assets:Checking                                  20 USD             20 USD
                                          Income:Dividend                                 -20 USD                  0
17-Jan-01 Commodities revalued            <Revalued>                                     2000 USD           2000 USD

@simonmichael

This comment has been minimized.

Copy link
Owner

commented Sep 6, 2019

Thanks for the testing. Good catch!

P 2001/01/01 A  1 B
P 2002/01/01 A  2 B

2000-01-01
  (a)  1 A

$ COLUMNS=80 ledger reg -V -e 2002-01-01
00-Jan-01 <Unspecified payee>   (a)                             1 A          1 A
01-Jan-01 Commodities revalued  <Revalued>                        0           B1
02-Jan-01 Commodities revalued  <Revalued>                       B1           B2
$ hledger-1.14 reg -w80 -V -e 2002-01-01
2000/01/01                      (a)                            2 B           2 B
$ hledger-1.15 reg -w80 -V -e 2002-01-01
2000/01/01                      (a)                            2 B           2 B
$ hledger-1.15.2 reg -w80 -V -e 2002-01-01
2000/01/01                      (a)                            1 B           1 B

As you say, in hledger 1.15.2 the report end date is exclusive with respect to price directives. Which is consistent with how it works when filtering transactions.. but different from previous hledger versions and Ledger. Which was not intended.

simonmichael added a commit that referenced this issue Sep 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.