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

hledger-1.2 print -B regression #551

Closed
simonmichael opened this Issue May 22, 2017 · 12 comments

Comments

Projects
None yet
3 participants
@simonmichael
Owner

simonmichael commented May 22, 2017

print seems to ignore -B as of hledger 1.2. Eg example 3 at http://hledger.org/manual.html#transaction-prices no longer works:

2009/1/1
  assets:foreign currency   €100
  assets:cash              $-135
> hledger-1.1 -f t.j print -B
2009/01/01
    assets:foreign currency          $135
    assets:cash                     $-135

> hledger-1.2 -f t.j print -B
2009/01/01
    assets:foreign currency          €100
    assets:cash                     $-135

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

simonmichael added a commit that referenced this issue May 23, 2017

doc: clarify some transaction price details
- remove inaccurate print examples
- note #551
- note Ledger's fixed price syntax
- note how posting order can affect -B
- edits for clarity
@cwarden

This comment has been minimized.

Show comment
Hide comment
@cwarden

cwarden May 30, 2017

Collaborator

@simonmichael, I used git bisect to identify 015b764 as commit that changed the behavior. Using --explicit with -B gives the hledger 1.1 output.

Collaborator

cwarden commented May 30, 2017

@simonmichael, I used git bisect to identify 015b764 as commit that changed the behavior. Using --explicit with -B gives the hledger 1.1 output.

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael May 30, 2017

Owner

Thanks! git bisect just found it here also, about the same time I found it manually, but good to practice with git bisect. I'm working on a fix.

Owner

simonmichael commented May 30, 2017

Thanks! git bisect just found it here also, about the same time I found it manually, but good to practice with git bisect. I'm working on a fix.

simonmichael added a commit that referenced this issue May 30, 2017

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael May 30, 2017

Owner

Well. I'm not smart enough to see a fix right now. Tracing this by code inspection and debugging was pretty hard; the code has grown rather complicated. I feel it goes wrong around (transient line links ahead) https://github.com/simonmichael/hledger/blob/master/hledger-lib/Hledger/Data/Journal.hs#L801 - print runs entriesReport which due to -B is converting all amounts to cost, but the p here is the original posting as written in the journal (with possibly implicit amounts & prices); we need the fully-explicit one, which has the inferred price.

Owner

simonmichael commented May 30, 2017

Well. I'm not smart enough to see a fix right now. Tracing this by code inspection and debugging was pretty hard; the code has grown rather complicated. I feel it goes wrong around (transient line links ahead) https://github.com/simonmichael/hledger/blob/master/hledger-lib/Hledger/Data/Journal.hs#L801 - print runs entriesReport which due to -B is converting all amounts to cost, but the p here is the original posting as written in the journal (with possibly implicit amounts & prices); we need the fully-explicit one, which has the inferred price.

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael May 30, 2017

Owner

PS and if we add the inferred price to the original posting as well in inferprice, it displays wrongly when you're not using -B. Maybe @ony will have an idea.

It's unfortunate that we had to add this copy of the posting (for more user-friendly print output); it's only natural that it will increase bugs and maintenance cost. Our transaction-posting knot-tying is already a nice source of hard to fix bugs relating to posting identity. I wonder if there's a way to do less copying and linking. Maybe just store some display flags to record how the original transaction was written ?

Owner

simonmichael commented May 30, 2017

PS and if we add the inferred price to the original posting as well in inferprice, it displays wrongly when you're not using -B. Maybe @ony will have an idea.

It's unfortunate that we had to add this copy of the posting (for more user-friendly print output); it's only natural that it will increase bugs and maintenance cost. Our transaction-posting knot-tying is already a nice source of hard to fix bugs relating to posting identity. I wonder if there's a way to do less copying and linking. Maybe just store some display flags to record how the original transaction was written ?

@ony

This comment has been minimized.

Show comment
Hide comment
@ony

ony Jun 4, 2017

Collaborator

@simonmichael having different types to represent data in user format (multiple ways to write same transaction) and in internal fully inferred one (only one representation) might simplify code a lot, I think.
Also, instead knot-tying it may make sense to build short-living ties only when we need to pass them around. That will allow easily distinguish functions that work only on posting level and which require whole transaction.

As for the CLI. It makes no sense to use print with -B and without --explicit. We may make dependency between these options and either imply --explicit on -B or report an error.
Note that there is multiple things we do to transform original journal. --explicit simply says - apply them all. We can go with partial applications but even then there might be implied transforms. Imagine:

2009/1/1
  assets:foreign currency    = €-100
  assets:cash              $-135

Should we print?

2009/1/1
  assets:foreign currency    $125 = €-100
  assets:cash              $-135
Collaborator

ony commented Jun 4, 2017

@simonmichael having different types to represent data in user format (multiple ways to write same transaction) and in internal fully inferred one (only one representation) might simplify code a lot, I think.
Also, instead knot-tying it may make sense to build short-living ties only when we need to pass them around. That will allow easily distinguish functions that work only on posting level and which require whole transaction.

As for the CLI. It makes no sense to use print with -B and without --explicit. We may make dependency between these options and either imply --explicit on -B or report an error.
Note that there is multiple things we do to transform original journal. --explicit simply says - apply them all. We can go with partial applications but even then there might be implied transforms. Imagine:

2009/1/1
  assets:foreign currency    = €-100
  assets:cash              $-135

Should we print?

2009/1/1
  assets:foreign currency    $125 = €-100
  assets:cash              $-135
@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Jun 4, 2017

Owner

Thanks for the comments. Which command line flags are you imagining in the example above ?

Hmm, are there really no legitimate cases where you might want to use -B but not --explicit ?

Anyway, one thing we know is that this should work:

$ hledger -f- print -B
2009/1/1
  assets:foreign currency   €100
  assets:cash              $-135
^D
2009/01/01
    assets:foreign currency          $135
    assets:cash                     $-135

It sounds like a less duplicative way of remembering the original journal entry format might make this easier.

Owner

simonmichael commented Jun 4, 2017

Thanks for the comments. Which command line flags are you imagining in the example above ?

Hmm, are there really no legitimate cases where you might want to use -B but not --explicit ?

Anyway, one thing we know is that this should work:

$ hledger -f- print -B
2009/1/1
  assets:foreign currency   €100
  assets:cash              $-135
^D
2009/01/01
    assets:foreign currency          $135
    assets:cash                     $-135

It sounds like a less duplicative way of remembering the original journal entry format might make this easier.

@ony

This comment has been minimized.

Show comment
Hide comment
@ony

ony Jun 7, 2017

Collaborator

Thanks for the comments. Which command line flags are you imagining in the example above ?

I referred to expected behavior from print with -B without --explicit flag. I.e. when implicit values are not requested to be present in output.

Hmm, are there really no legitimate cases where you might want to use -B but not --explicit ?

I have no idea. It looks like I never got a correct understanding of how -B works.
Previously I thought that it is similar to -V but instead of using price at the end of report it uses latest available price on the moment of posting.
For my journal this option is useless because I try to put main account (usually assets) as a first posting to see nice folding lines in vim.

@simonmichael what are use-cases for print with -B?
Do you think other reporting options like --date2 should affect output of print as well?

It sounds like a less duplicative way of remembering the original journal entry format might make this easier.

I don't think we duplicate much because of sharing thumbs/refs. For me it looks a bit hard to re-design our Amount-related types to properly catch "hidden" style and ensure that we always have value.

Collaborator

ony commented Jun 7, 2017

Thanks for the comments. Which command line flags are you imagining in the example above ?

I referred to expected behavior from print with -B without --explicit flag. I.e. when implicit values are not requested to be present in output.

Hmm, are there really no legitimate cases where you might want to use -B but not --explicit ?

I have no idea. It looks like I never got a correct understanding of how -B works.
Previously I thought that it is similar to -V but instead of using price at the end of report it uses latest available price on the moment of posting.
For my journal this option is useless because I try to put main account (usually assets) as a first posting to see nice folding lines in vim.

@simonmichael what are use-cases for print with -B?
Do you think other reporting options like --date2 should affect output of print as well?

It sounds like a less duplicative way of remembering the original journal entry format might make this easier.

I don't think we duplicate much because of sharing thumbs/refs. For me it looks a bit hard to re-design our Amount-related types to properly catch "hidden" style and ensure that we always have value.

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Jun 7, 2017

Owner

-B is documented at http://hledger.org/manual.html#cost. It converts based on a transaction price within the same transaction (only). -V converts based on an applicable market price at the report end date (only).

I don't have a specific use case for print -B ready to hand. It's just a general reporting option supported by most commands. Sometimes it is useful to see a transaction with amounts converted to the other commodity. Usually -B has the effect of reducing the number of commodities in a report.

Owner

simonmichael commented Jun 7, 2017

-B is documented at http://hledger.org/manual.html#cost. It converts based on a transaction price within the same transaction (only). -V converts based on an applicable market price at the report end date (only).

I don't have a specific use case for print -B ready to hand. It's just a general reporting option supported by most commands. Sometimes it is useful to see a transaction with amounts converted to the other commodity. Usually -B has the effect of reducing the number of commodities in a report.

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Jun 7, 2017

Owner

Just to follow on that last statement: "use -B and it will look right" is one of the most frequent recurring answers on #ledger over the years..

Owner

simonmichael commented Jun 7, 2017

Just to follow on that last statement: "use -B and it will look right" is one of the most frequent recurring answers on #ledger over the years..

@ony

This comment has been minimized.

Show comment
Hide comment
@ony

ony Jun 8, 2017

Collaborator

It's just a general reporting option supported by most commands.

"reporting" - that's my main point for questioning having it in print.

"use -B and it will look right"

This probably true for balances and other reports. But I doubt that if you write something in journal you expect to see in print ("Show transactions from the journal") different numbers than you've put there.

If you want to apply reporting options to output of print - then we should consider other reporting options as well like --date2. Shouldn't we?

I can see a benefit of that for print to debug what hledger actually "understand" from your journal with all that general options.

@simonmichael What you think about moving print without --explicit in a separate command source or query since it looks like print doing something different?

Collaborator

ony commented Jun 8, 2017

It's just a general reporting option supported by most commands.

"reporting" - that's my main point for questioning having it in print.

"use -B and it will look right"

This probably true for balances and other reports. But I doubt that if you write something in journal you expect to see in print ("Show transactions from the journal") different numbers than you've put there.

If you want to apply reporting options to output of print - then we should consider other reporting options as well like --date2. Shouldn't we?

I can see a benefit of that for print to debug what hledger actually "understand" from your journal with all that general options.

@simonmichael What you think about moving print without --explicit in a separate command source or query since it looks like print doing something different?

@ony ony self-assigned this Jun 11, 2017

@ony

This comment has been minimized.

Show comment
Hide comment
@ony

ony Jun 11, 2017

Collaborator

@simonmichael, if your only case is print -B you can use next patch:

From 1f787b2a177c1b2ef1cd65209bfbd8b8e32bd33b Mon Sep 17 00:00:00 2001
From: Mykola Orliuk <virkony@gmail.com>
Date: Sun, 11 Jun 2017 23:34:21 +0200
Subject: [PATCH] Implicit explicit on cost in print

Bring back behavior of hledger 1.1 for `print -B` until further
clarifications on `print` command.

Closes simonmichael/hledger#551
---
 hledger/Hledger/Cli/Print.hs | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/hledger/Hledger/Cli/Print.hs b/hledger/Hledger/Cli/Print.hs
index 4b88d88e..0b6e90af 100644
--- a/hledger/Hledger/Cli/Print.hs
+++ b/hledger/Hledger/Cli/Print.hs
@@ -46,6 +46,13 @@ printmode = (defCommandMode $ ["print"] ++ aliases) {
  }
   where aliases = []
 
+-- Check if explicit report were requested
+explicitOpt :: CliOpts -> Bool
+explicitOpt opts = or
+    [ boolopt "explicit" $ rawopts_ opts
+    , cost_ $ reportopts_ opts
+    ]
+
 -- | Print journal transactions in standard format.
 print' :: CliOpts -> Journal -> IO ()
 print' opts j = do
@@ -66,8 +73,8 @@ printEntries opts@CliOpts{reportopts_=ropts} j = do
 entriesReportAsText :: CliOpts -> EntriesReport -> String
 entriesReportAsText opts = concatMap (showTransactionUnelided . gettxn) 
   where
-    gettxn | boolopt "explicit" $ rawopts_ opts = id                   -- use the fully inferred/explicit txn
-           | otherwise                          = originalTransaction  -- use the original txn (more or less)
+    gettxn | explicitOpt opts = id                   -- use the fully inferred/explicit txn
+           | otherwise        = originalTransaction  -- use the original txn (more or less)
 
 -- Replace this transaction's postings with the original postings if any, but keep the
 -- current possibly rewritten account names.
-- 
2.11.0

Please note that I still see no justification for bringing back this behavior. When --explicit were introduced there were an option to use --implicit/--original instead. I.e. preserve original behavior and allow changing it with option.
Why new behavior of print output is valid and print -B is invalid?

Collaborator

ony commented Jun 11, 2017

@simonmichael, if your only case is print -B you can use next patch:

From 1f787b2a177c1b2ef1cd65209bfbd8b8e32bd33b Mon Sep 17 00:00:00 2001
From: Mykola Orliuk <virkony@gmail.com>
Date: Sun, 11 Jun 2017 23:34:21 +0200
Subject: [PATCH] Implicit explicit on cost in print

Bring back behavior of hledger 1.1 for `print -B` until further
clarifications on `print` command.

Closes simonmichael/hledger#551
---
 hledger/Hledger/Cli/Print.hs | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/hledger/Hledger/Cli/Print.hs b/hledger/Hledger/Cli/Print.hs
index 4b88d88e..0b6e90af 100644
--- a/hledger/Hledger/Cli/Print.hs
+++ b/hledger/Hledger/Cli/Print.hs
@@ -46,6 +46,13 @@ printmode = (defCommandMode $ ["print"] ++ aliases) {
  }
   where aliases = []
 
+-- Check if explicit report were requested
+explicitOpt :: CliOpts -> Bool
+explicitOpt opts = or
+    [ boolopt "explicit" $ rawopts_ opts
+    , cost_ $ reportopts_ opts
+    ]
+
 -- | Print journal transactions in standard format.
 print' :: CliOpts -> Journal -> IO ()
 print' opts j = do
@@ -66,8 +73,8 @@ printEntries opts@CliOpts{reportopts_=ropts} j = do
 entriesReportAsText :: CliOpts -> EntriesReport -> String
 entriesReportAsText opts = concatMap (showTransactionUnelided . gettxn) 
   where
-    gettxn | boolopt "explicit" $ rawopts_ opts = id                   -- use the fully inferred/explicit txn
-           | otherwise                          = originalTransaction  -- use the original txn (more or less)
+    gettxn | explicitOpt opts = id                   -- use the fully inferred/explicit txn
+           | otherwise        = originalTransaction  -- use the original txn (more or less)
 
 -- Replace this transaction's postings with the original postings if any, but keep the
 -- current possibly rewritten account names.
-- 
2.11.0

Please note that I still see no justification for bringing back this behavior. When --explicit were introduced there were an option to use --implicit/--original instead. I.e. preserve original behavior and allow changing it with option.
Why new behavior of print output is valid and print -B is invalid?

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Dec 31, 2017

Owner

Slight updates:

Re should --date2 affect print: I take your point but --date2 is a bad example, I consider it a misfeature so I'd answer no. Any other flags that would appear inconsistent here ?

I see that -B affects print if and only if -x is used. To me that's surprising and needs fixing one way or another. Currently we document -x as meaning "show all amounts explicitly", nothing more.

My "use -B and it will look right" comment was irrelevant here, ignore it.

I agree that we are using print for different purposes - showing raw journal content, showing semantic transactions as understood by hledger, adhoc reporting, troubleshooting. And that print -B doesn't make sense for 1 and 2. I think this multi-use policy is ok for the moment, not yet causing problems. Also there's the policy of letting common flags affect commands whenever it has some utility or reasonably non-confusing behaviour, for consistency. In this light I am leaning towards making print -B work again like it used to.

Owner

simonmichael commented Dec 31, 2017

Slight updates:

Re should --date2 affect print: I take your point but --date2 is a bad example, I consider it a misfeature so I'd answer no. Any other flags that would appear inconsistent here ?

I see that -B affects print if and only if -x is used. To me that's surprising and needs fixing one way or another. Currently we document -x as meaning "show all amounts explicitly", nothing more.

My "use -B and it will look right" comment was irrelevant here, ignore it.

I agree that we are using print for different purposes - showing raw journal content, showing semantic transactions as understood by hledger, adhoc reporting, troubleshooting. And that print -B doesn't make sense for 1 and 2. I think this multi-use policy is ok for the moment, not yet causing problems. Also there's the policy of letting common flags affect commands whenever it has some utility or reasonably non-confusing behaviour, for consistency. In this light I am leaning towards making print -B work again like it used to.

simonmichael added a commit that referenced this issue Dec 31, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment