From bb7d04c031e3d6d1e7a3e907179fa9358f91735b Mon Sep 17 00:00:00 2001 From: Stephen Morgan Date: Tue, 22 Jun 2021 00:29:31 +1000 Subject: [PATCH] lib,cli: No longer strip prices in journalApplyValuationFromOptsWith and mixedAmountApplyValuationAfterSumFromOptsWith (#1577). These were theoretically an efficiency improvement, but have been error-prone. We instead handle stripping prices at the point of consumption. --- hledger-lib/Hledger/Reports/MultiBalanceReport.hs | 5 +++-- hledger-lib/Hledger/Reports/ReportOptions.hs | 6 ++---- hledger/Hledger/Cli/Commands/Balance.hs | 2 -- hledger/Hledger/Cli/Commands/Close.hs | 9 +++++---- hledger/Hledger/Cli/Commands/Register.hs | 10 ++++------ 5 files changed, 14 insertions(+), 18 deletions(-) diff --git a/hledger-lib/Hledger/Reports/MultiBalanceReport.hs b/hledger-lib/Hledger/Reports/MultiBalanceReport.hs index 40cc7ab45..94e3f6f34 100644 --- a/hledger-lib/Hledger/Reports/MultiBalanceReport.hs +++ b/hledger-lib/Hledger/Reports/MultiBalanceReport.hs @@ -407,8 +407,9 @@ displayedAccounts ReportSpec{rsQuery=query,rsOpts=ropts} valuedaccts || not (isZeroRow balance amts)) -- Throw out anything with zero balance where d = accountNameLevel name - balance | ALTree <- accountlistmode_ ropts, d == depth = aibalance - | otherwise = aebalance + balance | ALTree <- accountlistmode_ ropts, d == depth = maybeStripPrices . aibalance + | otherwise = maybeStripPrices . aebalance + where maybeStripPrices = if show_costs_ ropts then id else mixedAmountStripPrices -- Accounts interesting because they are a fork for interesting subaccounts interestingParents = dbg5 "interestingParents" $ case accountlistmode_ ropts of diff --git a/hledger-lib/Hledger/Reports/ReportOptions.hs b/hledger-lib/Hledger/Reports/ReportOptions.hs index 5fd1a730b..c8e854806 100644 --- a/hledger-lib/Hledger/Reports/ReportOptions.hs +++ b/hledger-lib/Hledger/Reports/ReportOptions.hs @@ -506,10 +506,9 @@ journalApplyValuationFromOpts rspec j = -- | Like journalApplyValuationFromOpts, but takes PriceOracle as an argument. journalApplyValuationFromOptsWith :: ReportSpec -> Journal -> PriceOracle -> Journal journalApplyValuationFromOptsWith rspec@ReportSpec{rsOpts=ropts} j priceoracle = - journalMapPostings (valuation . maybeStripPrices) $ costing j + journalMapPostings valuation $ costing j where valuation p = maybe id (postingApplyValuation priceoracle styles (periodEnd p) (rsToday rspec)) (value_ ropts) p - maybeStripPrices = if show_costs_ ropts then id else postingStripPrices costing = case cost_ ropts of Cost -> journalToCost NoCost -> id @@ -528,12 +527,11 @@ mixedAmountApplyValuationAfterSumFromOptsWith :: ReportOpts -> Journal -> PriceO -> (DateSpan -> MixedAmount -> MixedAmount) mixedAmountApplyValuationAfterSumFromOptsWith ropts j priceoracle = case valuationAfterSum ropts of - Just mc -> \span -> valuation mc span . maybeStripPrices . costing + Just mc -> \span -> valuation mc span . costing Nothing -> const id where valuation mc span = mixedAmountValueAtDate priceoracle styles mc (maybe err (addDays (-1)) $ spanEnd span) where err = error "mixedAmountApplyValuationAfterSumFromOptsWith: expected all spans to have an end date" - maybeStripPrices = if show_costs_ ropts then id else mixedAmountStripPrices costing = case cost_ ropts of Cost -> styleMixedAmount styles . mixedAmountCost NoCost -> id diff --git a/hledger/Hledger/Cli/Commands/Balance.hs b/hledger/Hledger/Cli/Commands/Balance.hs index 2fc6a4d3e..c51da579d 100644 --- a/hledger/Hledger/Cli/Commands/Balance.hs +++ b/hledger/Hledger/Cli/Commands/Balance.hs @@ -654,8 +654,6 @@ balanceOpts :: Bool -> ReportOpts -> AmountDisplayOpts balanceOpts isTable ReportOpts{..} = oneLine { displayColour = isTable && color_ , displayMaxWidth = if isTable && not no_elide_ then Just 32 else Nothing - , displayPrice = True -- multiBalanceReport strips prices from Amounts if they are not being used, - -- so we can display prices here without fear. } tests_Balance = tests "Balance" [ diff --git a/hledger/Hledger/Cli/Commands/Close.hs b/hledger/Hledger/Cli/Commands/Close.hs index 49f2129fa..b700b4175 100755 --- a/hledger/Hledger/Cli/Commands/Close.hs +++ b/hledger/Hledger/Cli/Commands/Close.hs @@ -83,8 +83,9 @@ close CliOpts{rawopts_=rawopts, reportspec_=rspec} j = do explicit = boolopt "explicit" rawopts -- the balances to close - (acctbals,_) = balanceReport rspec_ j - totalamt = maSum $ map (\(_,_,_,b) -> b) acctbals + (acctbals',_) = balanceReport rspec_ j + acctbals = map (\(a,_,_,b) -> (a, if show_costs_ ropts then b else mixedAmountStripPrices b)) acctbals' + totalamt = maSum $ map snd acctbals -- since balance assertion amounts are required to be exact, the -- amounts in opening/closing transactions should be too (#941, #1137) @@ -111,7 +112,7 @@ close CliOpts{rawopts_=rawopts, reportspec_=rspec} j = do ++ [posting{paccount=closingacct, pamount=mixedAmount $ precise b} | interleaved] | -- get the balances for each commodity and transaction price - (a,_,_,mb) <- acctbals + (a,mb) <- acctbals , let bs = amounts mb -- mark the last balance in each commodity with True , let bs' = concat [reverse $ zip (reverse bs) (True : repeat False) @@ -137,7 +138,7 @@ close CliOpts{rawopts_=rawopts, reportspec_=rspec} j = do ] ++ [posting{paccount=openingacct, pamount=mixedAmount . precise $ negate b} | interleaved] - | (a,_,_,mb) <- acctbals + | (a,mb) <- acctbals , let bs = amounts $ normaliseMixedAmount mb -- mark the last balance in each commodity with the unpriced sum in that commodity (for a balance assertion) , let bs' = concat [reverse $ zip (reverse bs) (Just commoditysum : repeat Nothing) diff --git a/hledger/Hledger/Cli/Commands/Register.hs b/hledger/Hledger/Cli/Commands/Register.hs index 8cf3e24b6..cf19964be 100644 --- a/hledger/Hledger/Cli/Commands/Register.hs +++ b/hledger/Hledger/Cli/Commands/Register.hs @@ -89,8 +89,8 @@ postingsReportItemAsCsvRecord (_, _, _, p, b) = [idx,date,code,desc,acct,amt,bal VirtualPosting -> wrap "(" ")" _ -> id -- Since postingsReport strips prices from all Amounts when not used, we can display prices. - amt = wbToText . showMixedAmountB oneLine{displayPrice=True} $ pamount p - bal = wbToText $ showMixedAmountB oneLine{displayPrice=True} b + amt = wbToText . showMixedAmountB oneLine $ pamount p + bal = wbToText $ showMixedAmountB oneLine b -- | Render a register report as plain text suitable for console output. postingsReportAsText :: CliOpts -> PostingsReport -> TL.Text @@ -104,7 +104,7 @@ postingsReportAsText opts items = TB.toLazyText $ foldMap first3 linesWithWidths amtwidth = maximumStrict $ 12 : widths (map itemamt items) balwidth = maximumStrict $ 12 : widths (map itembal items) -- Since postingsReport strips prices from all Amounts when not used, we can display prices. - widths = map wbWidth . concatMap (showMixedAmountLinesB oneLine{displayPrice=True}) + widths = map wbWidth . concatMap (showMixedAmountLinesB oneLine) itemamt (_,_,_,Posting{pamount=a},_) = a itembal (_,_,_,_,a) = a @@ -187,9 +187,7 @@ postingsReportItemAsText opts preferredamtwidth preferredbalwidth (mdate, mendda _ -> (id,acctwidth) amt = showamt $ pamount p bal = showamt b - -- Since postingsReport strips prices from all Amounts when not used, we can display prices. - showamt = showMixedAmountLinesB oneLine{displayColour=color_, displayPrice=True} - where ReportOpts{..} = rsOpts $ reportspec_ opts + showamt = showMixedAmountLinesB oneLine{displayColour=color_ . rsOpts $ reportspec_ opts} -- Since this will usually be called with the knot tied between this(amt|bal)width and -- preferred(amt|bal)width, make sure the former do not depend on the latter to avoid loops. thisamtwidth = maximumDef 0 $ map wbWidth amt