slaesvuo/hledger
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

csv: undo failed #1192 fix, causing "_unknown_" accounts; refactor

Browse Source 
Restore the old logic, while also trying to clarify this
transaction-generating code, and add some tests.
See also #1198.
This commit is contained in:
Simon Michael 2020-02-26 21:46:11 -08:00
parent fb788a64e8
commit 58bb0df5ab
2 changed files with 94 additions and 50 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

99
hledger-lib/Hledger/Read/CsvReader.hs
View File

@ -798,46 +798,46 @@ transactionFromCsvRecord sourcepos rules record = t
] ]
-- Default account names to use when one is not set. -- Default account names to use when one is not set.
-- The first one is chosen by default, and sometimes gets replaced later
-- by the other when appropriate.
unknownExpenseAccount = "expenses:unknown" unknownExpenseAccount = "expenses:unknown"
unknownIncomeAccount = "income:unknown" unknownIncomeAccount = "income:unknown"
-- A temporary placeholder for the unknown account name, which
-- gets replaced by one of the above based on the amount's sign.
-- This is a value hopefully never chosen by users (cf #1192).
unknownPlaceholderAccount = "_unknown_"
parsePosting' :: String -> JournalFieldName -> JournalFieldName -> JournalFieldName -> JournalFieldName -> JournalFieldName -> JournalFieldName -> Maybe (String, Posting)
parsePosting' number accountFld amountFld amountInFld amountOutFld balanceFld commentFld = parsePosting' number accountFld amountFld amountInFld amountOutFld balanceFld commentFld =
let currency = maybe (fromMaybe "" mdefaultcurrency) render $ let currency = maybe (fromMaybe "" mdefaultcurrency) render $
(mfieldtemplate ("currency"++number) `or `mfieldtemplate "currency") (mfieldtemplate ("currency"++number) `or `mfieldtemplate "currency")
amount = chooseAmount rules record currency amountFld amountInFld amountOutFld mamount = chooseAmount rules record currency amountFld amountInFld amountOutFld
account' = ((T.pack . render) <$> (mfieldtemplate accountFld maccount' = ((T.pack . render) <$>
`or` mdirective ("default-account" ++ number))) (mfieldtemplate accountFld `or` mdirective ("default-account" ++ number)))
balance = (parsebalance currency number.render) =<< mfieldtemplate balanceFld mbalance = (parsebalance currency number.render) =<< mfieldtemplate balanceFld
comment = T.pack $ maybe "" render $ mfieldtemplate commentFld comment = T.pack $ maybe "" render $ mfieldtemplate commentFld
account = maccount :: Maybe AccountName =
case account' of case maccount' of
-- account is set to "" - suppress posting -- accountN is set to the empty string - this posting will be suppressed
Just "" -> Nothing Just "" -> Nothing
-- account is set -- accountN is set
Just account -> Just account Just a -> Just a
-- accountN is unset
Nothing -> Nothing ->
case (amount, balance) of case (mamount, mbalance) of
-- account is not set, but an amount is set (or implied by -- amountN is set, or implied by balanceN - accountN will be
-- balance assignment) - use "unknown" account. -- set to the unknown account (expenses:unknown, for now)
(Just _, _ ) -> Just unknownPlaceholderAccount (Just _, _) -> Just unknownExpenseAccount
(_, Just _) -> Just unknownPlaceholderAccount (_, Just _) -> Just unknownExpenseAccount
-- no account, no amount -- amountN is also unset, this posting will be suppressed
(Nothing, Nothing) -> Nothing (Nothing, Nothing) -> Nothing
in in
-- if there's an account N, make a posting N -- if there's an account N, make a posting N
case account of case maccount of
Nothing -> Nothing Nothing -> Nothing
Just account -> Just acct ->
Just $ (number, posting {paccount=accountNameWithoutPostingType account Just (number, posting{paccount = accountNameWithoutPostingType acct
, pamount=fromMaybe missingmixedamt amount ,pamount = fromMaybe missingmixedamt mamount
,ptransaction = Just t ,ptransaction = Just t
, pbalanceassertion=toAssertion <$> balance ,pbalanceassertion = toAssertion <$> mbalance
,pcomment = comment ,pcomment = comment
, ptype = accountNamePostingType account}) ,ptype = accountNamePostingType acct})
parsePosting number = parsePosting number =
parsePosting' number parsePosting' number
@ -870,31 +870,36 @@ transactionFromCsvRecord sourcepos rules record = t
postings = postings =
case postings' of case postings' of
-- To be compatible with the behavior of the old code which allowed two postings only, we enforce -- pre-1.16 compatibility: when rules generate just one posting, and
-- second posting when rules generated just first of them, and posting is of type that should be balanced. -- it's a type that needs to be balanced, generate the second posting
[("1",posting1)] -> -- to balance it, with appropriate unknown account name
case ptype posting1 of [("1",p1)] ->
VirtualPosting -> [posting1] if ptype p1 == VirtualPosting
_ -> then [p1]
[posting1,improveUnknownAccountName (posting{paccount=unknownPlaceholderAccount, pamount=costOfMixedAmount(-(pamount posting1)), ptransaction=Just t})] else [p1, p2]
-- When we have strictly first and second posting, but second posting does not have amount, we fill it in. where
[("1",posting1),("2",posting2)] -> p2 = improveUnknownAccountName $
case (pamount posting1 == missingmixedamt , pamount posting2 == missingmixedamt) of nullposting{paccount=unknownExpenseAccount
(False, True) -> [posting1, improveUnknownAccountName (posting2{pamount=costOfMixedAmount(-(pamount posting1))})] ,pamount=costOfMixedAmount (-pamount p1)
_ -> [posting1, posting2] ,ptransaction=Just t}
-- pre-1.16 compatibility: when rules generate exactly two postings,
-- and the second has no amount, give it the balancing amount, and
-- refine the account name if it's unknown
[("1",p1), ("2",p2)] ->
case (pamount p1 == missingmixedamt , pamount p2 == missingmixedamt) of
(False, True) -> [p1, improveUnknownAccountName $ p2{pamount=costOfMixedAmount(-(pamount p1))}]
_ -> [p1, p2]
-- otherwise, refine any unknown account names in all postings
_ -> map (improveUnknownAccountName . snd) postings' _ -> map (improveUnknownAccountName . snd) postings'
where where
improveUnknownAccountName p = -- If this posting has the "expenses:unknown" account name, maybe
if paccount p == unknownPlaceholderAccount -- replace that with "income:unknown" now that we know the amount's sign.
then case isNegativeMixedAmount (pamount p) of improveUnknownAccountName p@Posting{..}
-- amount is negative, call it an income | paccount == unknownExpenseAccount
Just True -> p{paccount = unknownIncomeAccount} && fromMaybe False (isNegativeMixedAmount pamount) = p{paccount=unknownIncomeAccount}
-- amount is positive, or in multiple commodities so we | otherwise = p
-- can't tell, call it an expense
_ -> p{paccount = unknownExpenseAccount}
else p
-- build the transaction -- build the transaction
t = nulltransaction{ t = nulltransaction{

39
tests/csv.test
View File

@ -574,6 +574,45 @@ $ ./hledger-csv desc:one
>=0 >=0
# 28. choose unknown account names correctly when no account name is set
# and backwards-compatibly generating two postings.
<
2020-01-01, 1,
RULES
fields date, amount,
$ ./hledger-csv
2020-01-01
expenses:unknown 1
income:unknown -1
>=0
# 29. choose unknown account name correctly when only account1 is set
# and backwards-compatibly generating second posting.
<
2020-01-01, 1, a
RULES
fields date, amount, account1
$ ./hledger-csv
2020-01-01
a 1
income:unknown -1
>=0
# 30. leave unknown account name as-is when explicitly set by user (#1192).
<
2020-01-01, 1, a
RULES
fields date, amount, account1
account2 expenses:unknown
$ ./hledger-csv
2020-01-01
a 1
expenses:unknown -1
>=0
## . ## .
#< #<
#$ ./hledger-csv #$ ./hledger-csv