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

Make transaction modifiers work correctly with inferred amounts #900

Merged
merged 5 commits into from Oct 12, 2018

Conversation

Projects
None yet
3 participants
@jkr
Contributor

jkr commented Oct 11, 2018

This solves #893 by having journalFinalise run twice with automated transactions: the first run takes care of reordering and inferring amounts (but does not check assertions), the second does not reorder. It was necessary to make reordering a boolean flag in journalFinalise. Note that if --auto is not specified, the finalization is only run one time, as before.

jkr added some commits Oct 11, 2018

Journal: make reordering optional in `journalFinalise`
Currently `journalFinalise` always reverses the order of
entries. However, if there are automated transactions, we might need
to run it twice. This adds a boolean flag to make reordering
optional. This will be used in the `parseAndFinaliseJournal`
functions.
read: Integrate transaction modifiers with journal finalization
Currently, automated transactions are added before the journal is
finalized. This means that no inferred values will be picked up. We
change the procedure, if `auto_` is set, to

 1. first run `journalFinalise` without assertion checking (assertions
    might be wrong until automated transactions), but with reordering
 2. Insert transaction modifiers
 3. Run `journalFinalise` again, this time with assertion checking as
    set in the options, and without reordering.

If `auto_` is not set, all works as before.

Closes: #893
@simonmichael

Also, there's a chance --auto could be always-on in future. Hopefully this doesn't have much performance impact, but we should probably think about/test that a little more.

,jperiodictxns = reverse $ jperiodictxns j -- NOTE: see addPeriodicTransaction
,jmarketprices = reverse $ jmarketprices j -- NOTE: see addMarketPrice
})
journalFinalise :: ClockTime -> FilePath -> Text -> Bool -> Bool -> ParsedJournal -> Either String Journal

This comment has been minimized.

@simonmichael

simonmichael Oct 11, 2018

Owner

Could you move/copy the nice commit doc explanation into the haddock.

}
else j
in journalTieTransactions <$>
(journalBalanceTransactions assrt $ journalApplyCommodityStyles j')

This comment has been minimized.

@simonmichael

simonmichael Oct 11, 2018

Owner

Bikeshedding: maybe confining the effect of reorder to a smaller block of code makes it clearer ?

journalFinalise t path txt reorder assrt j@Journal { jfiles = fs } =
  journalTieTransactions
  <$> (journalBalanceTransactions assrt $
       journalApplyCommodityStyles j{
          jfiles            = (path, txt) : mayberev fs
          , jlastreadtime     = t
          , jdeclaredaccounts = mayberev $ jdeclaredaccounts j
          , jtxns             = mayberev $ jtxns j         -- NOTE: see addTransaction
          , jtxnmodifiers     = mayberev $ jtxnmodifiers j -- NOTE: see addTransactionModifier
          , jperiodictxns     = mayberev $ jperiodictxns j -- NOTE: see addPeriodicTransaction
          , jmarketprices     = mayberev $ jmarketprices j -- NOTE: see addMarketPrice
          })
  where mayberev = if reorder then reverse else id

This comment has been minimized.

@jkr

jkr Oct 12, 2018

Contributor

Yeah -- I like this. But in this case, we'd be adding (path,txt) to jfiles twice in the --auto case, right?

This comment has been minimized.

@simonmichael

simonmichael Oct 12, 2018

Owner

True. It's also setting jlastreadtime. "reorder" doesn't quite cover what it's about. "finallyfinalise" ? Split this function into two ?

This comment has been minimized.

@simonmichael

simonmichael Oct 12, 2018

Owner

I don't see exactly how to clarify it right now.. if you don't see a way, it's ok to leave it how it was.

This comment has been minimized.

@jkr

jkr Oct 12, 2018

Contributor

journalFinalise is only used in Read.Common's parseAndFinaliseJournal (and the primed version). Since it's a single-use function, I wonder if combining all its steps is creating more confusion. I.e., just combine them in parseAndFinaliseJounal as we need them, instead of adding a bunch of boolean flags in journalFinalise. (The boolean flags just function to break it apart, opaquely, the one time we use it.)

This comment has been minimized.

@simonmichael

This comment has been minimized.

@jkr

jkr Oct 12, 2018

Contributor

See 6a4ece6 below for a try at this splitting up (docs aren't added, and it isn't added to the primed function).

Pros: more explicit what's going on; no opaque boolean flags; no running unnecessary steps
Cons: at first blush, a more confusing parseAndFinaliseJournal function

in
case fj of
Right j -> return j
Left e -> throwError e

This comment has been minimized.

@simonmichael

simonmichael Oct 11, 2018

Owner

IIRC this code is already a bit hard to grok, so I don't love that we are adding more complexity.

The commit doc is clearer than the code comment, perhaps you can move more of it into the code (as haddock or ordinary comment).

I really don't love the idea of duplicating this twice. Or that we have these two similar looking functions, one with no docs. I wonder if we can clean this up.

@simonmichael

This comment has been minimized.

Owner

simonmichael commented Oct 12, 2018

(It seems like parseAndFinaliseJournal' should go away, it's only used by the timeclock and timedot readers. Right @awjchen ?)

@jkr

This comment has been minimized.

Contributor

jkr commented Oct 12, 2018

(It seems like parseAndFinaliseJournal' should go away, it's only used by the timeclock and timedot readers. Right @awjchen ?)

Per my ack search when I came across it, that's all.

read: only run finalise twice if there are modifiers
Previously we ran if `--auto` was set. But this adds a small
performance hit if `--auto` becomes default. Now we only run twice if
there are transactionModifiers AND `--auto` is set. So even if auto is
specified, there will be no penalty if there are no modifiers.
@jkr

This comment has been minimized.

Contributor

jkr commented Oct 12, 2018

I added a commit (8cbead6) which only does the double run if --auto is set AND the jtxnmodifiers is non-null. So even if auto is default, there will be no performance hit unless there are modifiers to apply.

(Informal tests showed a slight performance hit when using --auto before, though I haven't run it on large enough journals to know how it increases with size.)

@@ -297,9 +297,19 @@ parseAndFinaliseJournal' parser iopts f txt = do
runFin reorder ignore = journalFinalise t f txt reorder ignore

This comment has been minimized.

@simonmichael

simonmichael Oct 12, 2018

Owner

Did you mean to stop calling journalFinalise here, and drop that function ?

This comment has been minimized.

@jkr

jkr Oct 12, 2018

Contributor

I was just adding it to the unprimed version as an example. If it seems like a good idea, I'll go through, add docs to the smaller functions, and integrate it into the primed version. (BTW, I still don't understand the primed one -- it seems to relate to ExceptT error catching, but I note that the program compiles fine with all tests passing if it's replaced in TimeClock and TimeDot with the unprimed one.)

This comment has been minimized.

@jkr

jkr Oct 12, 2018

Contributor

Oh -- oops. I see what you mean. One moment.

journal: split up the parts of journalFinalise, and use them as needed.
`journalFinalise` is only used in the `parseAndFinaliseJournal`
functions, but it needs to be run differently at different stages when
transaction modifiers are applied. This change breaks it into smaller
functions, and uses those smaller parts in `parseAndFinaliseJournal`
as needed.

@jkr jkr force-pushed the jkr:modifiers_fix_2 branch from 6a4ece6 to e84bd70 Oct 12, 2018

@simonmichael

This comment has been minimized.

Owner

simonmichael commented Oct 12, 2018

I was just adding it to the unprimed version as an example. If it seems like a good idea, I'll go through, add docs to the smaller functions, and integrate it into the primed version.

Yep good idea, it looks much clearer.

(BTW, I still don't understand the primed one -- it seems to relate to ExceptT error catching, but I note that the program compiles fine with all tests passing if it's replaced in TimeClock and TimeDot with the unprimed one.)

ErroringJournalParser is a variant of JournalParser that can abort parsing by throwing an exception. It's our most general and featureful kind of journal parser. Some months ago we removed it thinking it was unneeded, and it has recently been re-added.

@simonmichael simonmichael merged commit 53b3e2b into simonmichael:master Oct 12, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@awjchen

This comment has been minimized.

Collaborator

awjchen commented Oct 12, 2018

(BTW, I still don't understand the primed one -- it seems to relate to ExceptT error catching, but I note that the program compiles fine with all tests passing if it's replaced in TimeClock and TimeDot with the unprimed one.)

(It seems like parseAndFinaliseJournal' should go away, it's only used by the timeclock and timedot readers. Right @awjchen ?)

It's true that one can replace parseAndFinaliseJournal' with parseAndFinaliseJournal in TimeClock (and Timedot). This is because the TimeClock parsers are defined for all JournalParser m, while ErroringJournalParser n is just a synonym for JournalParser m where m = ExceptT FinalParseError n.

But using parseAndFinaliseJournal instead of parseAndFinaliseJournal' introduces an ExceptT layer that is subsequently ignored. By using parseAndFinaliseJournal' we are being more specific, using only what we need. For this reason, I would prefer to keep parseAndFinaliseJournal', all else equal; however, more practically, I don't see anything wrong with getting rid of it, especially if there is some reason to do so.

@simonmichael

This comment has been minimized.

Owner

simonmichael commented Oct 16, 2018

A practical reason would be getting rid of duplicated code and one more type folks don't have to think about. (We could also factor out the common code). I think there's no harm in standardising and using the more powerful type for the time parsers even though they don't make use of it yet.

@awjchen

This comment has been minimized.

Collaborator

awjchen commented Oct 16, 2018

Agreed, those sound like good reasons to me.

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