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

Don't allow assignments for accounts in transaction modifiers. #912

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@jkr
Contributor

jkr commented Oct 18, 2018

As discussed in comments for #908, this throws an error if there is an attempt to assign to an account that is also present in a transaction modifier.

Note that this currently works whether or not --auto is set, so that it doesn't produce a regression in rewrites (which also use the jtxnmodifier list).

jkr added some commits Oct 18, 2018

@simonmichael

The code looks good.

I don't have a firm enough grasp of the problem you ran into, whether it happens in practice etc. The journal from your test works fine with hledger 1.11. Shouldn't it break in the way you reported ?

I'm still a little unsure of the benefit. Is the cure (preemptively disallowing a surprising interaction) worse than the disease (added complexity, this plus the future stuff/corner cases you mention) ?

@simonmichael

This comment has been minimized.

Owner

simonmichael commented Oct 18, 2018

Oh I see. It works with hledger-1.11, but not with master, because of your earlier fix. Please correct me if I'm confused.

@jkr

This comment has been minimized.

Contributor

jkr commented Oct 19, 2018

Right -- it didn't use to be a problem (in some cases) because it always applied transaction modifiiers before it did the other finalization, which would miss inference and other things. But fixing that (and allowing inference before modification) creates this new problem, where assignment (done before modification) is wrong after modification.

The errors for this now are opaque: it's hard to figure out why the assignment failed. This is an attempt to explain to the user that assignment and modification don't play well (or predictably) together.

I'm not totally sure if the cure is better or worse than the disease -- but it came out of errors when trying to convert my ledger-cli journal to hledger, and discovering that something that worked there didn't work here, without being able to figure out why.

@simonmichael

This comment has been minimized.

Owner

simonmichael commented Oct 30, 2018

Sorry for the delay on this. What's the current situation as you see it ?

@jkr

This comment has been minimized.

Contributor

jkr commented Oct 31, 2018

I guess it seems more like a form of preventative error reporting. If you run balance assignments with automated transactions, you'll either have unexpected results on the --auto side (as with hledger) or on the --actual side (as with ledger-cli). So if folks do it, we could either do this, and refuse to do it, or error out with unbalanced transactions. I'm honestly not sure which is better.

Not a big deal for me, at this point, because I got in the habit of not using balance assignments -- just assertions, which is better because it's more explicit anyway. But it does seem like it might catch people coming over from ledger-cli, and explain why a journal that worked over there doesn't work over here.

There might also be other ways to do it. If you ran modifiers transaction-by-transactiion during the first finalization, instead of mapping it over after, that might allow for better control over this -- perhaps making the error unnecessary.

So, in answer to your question... maybe, not sure?

@simonmichael

This comment has been minimized.

Owner

simonmichael commented Dec 2, 2018

@jkr I've been reconstructing the history here as part of writing changelogs. Does the following look right ?

In hledger 1.11-, transaction modifier (auto postings) rules ran before missing amounts were inferred. This meant amount multipliers could generate too many missing-amount postings, making the transaction unbalanceable. (#893)

This was resolved by doing amount inference (and balance assignments & balance assertions; these three are done together) earlier, before transaction modifiers. (#900, #903)

We found a new issue caused by this change: now balance assignments are generated so as to satisfy balance assertions with the unmodified transactions, but if transaction modifiers add additional postings to those accounts, the assertions can fail. (#908)

The current proposed fix is to track which accounts can be affected by transaction modifier rules, and disallow balance assignments on those accounts. (#912)

@simonmichael

This comment has been minimized.

Owner

simonmichael commented Dec 2, 2018

If so, I guess I'm inclined to merge this. It seems better to have this limitation than the one in hledger 1.11. What do you think @adept ?

I wondered if we could infer missing amounts, then apply transaction modifiers, then do balance assignments and assertions. But I have the impression/recollection that missing amounts/balance assignments/balance assertions are interdependent and must be done as a single step.

@simonmichael

This comment has been minimized.

Owner

simonmichael commented Dec 2, 2018

Merged with some fixups (1389a64). Thanks!

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