Skip to content
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

Exact assertions & multi commodity balances #871

Closed
wants to merge 34 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
71d86f7
More inclusive .gitignore
ag-eitilt Sep 20, 2018
e430f40
Add Amount wrapper to allow extending assertions
ag-eitilt Sep 20, 2018
d6bfc1b
Add the required parser, and reconnect everything
ag-eitilt Sep 20, 2018
259f185
Realized the single-commodity assertions could be subsumed
ag-eitilt Sep 20, 2018
df966ed
mamountp tests
ag-eitilt Sep 20, 2018
a34950f
Use a plus sign rather than a comma to separate amounts
ag-eitilt Sep 21, 2018
f89d198
Update the CLI to reflect the library change
ag-eitilt Sep 21, 2018
893ca13
Fixes for cabal new-build
ag-eitilt Sep 21, 2018
5c6c0d0
Merge infrastructure fixes into assertions
ag-eitilt Sep 21, 2018
e3ecb30
Name assertion fields and take first steps with assertion flags
ag-eitilt Sep 21, 2018
d5063a8
Fix transaction display in errors
ag-eitilt Sep 21, 2018
3752e08
Update name to better reflect what it does now
ag-eitilt Sep 21, 2018
7415279
Complete the addition/subtraction pair
ag-eitilt Sep 21, 2018
55883aa
Fix not checking zero balances through MixedAmount optimizing them away
ag-eitilt Sep 21, 2018
df9755f
Allow brackets for power and compatibility with ledger
ag-eitilt Sep 21, 2018
3696bee
Fix bug with overzealous commodity parsing
ag-eitilt Sep 21, 2018
03c6092
Merge remote-tracking branch 'upstream/master'
ag-eitilt Sep 21, 2018
ce58572
Merge branch 'master' into multi-commodity
ag-eitilt Sep 21, 2018
d7f9028
Ensure inferring postings from assertions works
ag-eitilt Sep 21, 2018
f091455
Begin solving multipliers by multiplying them as they're read
ag-eitilt Sep 22, 2018
b9b09ee
Complete parse-time support for multipliers
ag-eitilt Sep 22, 2018
20a8cac
Fix various subtle issues with parse tests
ag-eitilt Sep 24, 2018
9aa3238
Fix commodity assertions being treated as exact with empty postings
ag-eitilt Sep 25, 2018
7b7160e
Add tests for amount expressions and exact assertions
ag-eitilt Sep 27, 2018
ae2ea34
Reduce duplication in multiplier code
ag-eitilt Sep 28, 2018
b3dbe88
Merge remote-tracking branch 'upstream/master' into multi-commodity
ag-eitilt Sep 28, 2018
edb9ba7
Little note before stopping work for the night
ag-eitilt Sep 28, 2018
f6f047e
Fix last test errors from previous refactor
ag-eitilt Sep 28, 2018
3be314b
Ensure quantity multipliers don't convert to the default commodity
ag-eitilt Sep 28, 2018
1a9fccc
These didn't actually have to be hardcoded
ag-eitilt Sep 28, 2018
2add5c2
Include instances for other package in project
ag-eitilt Sep 28, 2018
34ebedb
Test multiplying by multiple commodities
ag-eitilt Sep 29, 2018
7730f4a
Merge remote-tracking branch 'upstream/master' into multi-commodity
ag-eitilt Oct 4, 2018
a577404
First draft of docs
ag-eitilt Oct 4, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions site/contributors.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ hledger is brought to you by:
- Roman Cheplyaka - "chart" command, "add" command improvements
- Michael Snoyman - some additions to the Yesod web interface
- Marko Kocić - hlint cleanup
- Samuel May - amount expressions, assertion types

Developers who have not yet signed the contributor agreement:

Expand Down
65 changes: 62 additions & 3 deletions site/doc/1.11/journal.md
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,38 @@ when an amountless posting is balanced using a price's commodity, or
when -V is used.) If you find this causing problems, set the desired
format with a commodity directive.

#### Amount Expressions

An amount may also be comprised of multiple parts joined by basic
arithmetic operators; in this case, each individual value follows the
structure described above, while the joins allow:

- addition, subtraction, and multiplication using the standard ASCII
operators (`+`, `-`, `*`), either adjacent to the amount(s) or
separated by spaces
- parenthesised (sub-)expressions, to be evaluated before any
operations outside them. Note, however, that all commodity symbols
must be attached to an amount inside the parentheses: `($10 * 0.2)`
works, `$(10 * 0.2)` does not.
- multiple commodities in the same expression

Multiplication of commodities is handled a bit differently than may be
expected; if the amount to the right of the operator includes an
explicit commodity, that commodity replaces any to the left for
compatibility with [automated postings](#automated-postings). If that
right value includes multiple commodities itself (parenthesised to obey
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should discard commodities. Can we instead disallow multiplying by amounts, ie only multiplying by a number will be legal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could, and that was actually my first plan, but then I looked into the transaction modifiers and found that this is exactly how they currently operate. It's undocumented behaviour, so reverting that wouldn't as bad as it could be, but I was afraid someone else might have figured that out and was relying on that in their ledgers -- I can come up with a scheme managing gift cards/certificates (via different commodities) easily enough that that I'm sure there's more realistic usages out there as well.

I can probably rework the parsing to enforce that distinction, but we really don't want to have one type of multiplication at the beginning of transaction modifiers and another in expressions. Is disallowing multiplying by amounts worth potentially breaking journals? I obviously don't think so (it might not be what's immediately expected, but I feel it wouldn't be particularly tricky for users to get the hang of), but I'm not so strongly strongly attached to that that I'll argue this any further, if you still prefer it. Give the word and I'll add it to the cleanup list.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into the transaction modifiers and found that this is exactly how they currently operate. It's undocumented behaviour

You're right! I'd rather enforce that the multiplier is a commodityless number for now. Quietly discarding information from one side of an expression is something we should avoid. I'll clarify this in docs. I agree that it should work the same in transaction modifiers and in amount expressions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First take of it at #913, pending further discussion.

the order of operations), the left value is multiplied by each
individually.

As examples, with the value each expression simplifies to (if
applicable):

`$47.18-$7.13` -> `$40.05`
`$47.18 - ($20 + $7.13)` -> `$20.05`
`$6.10 + £4.35 * 0.8 €` -> `$6.10 + 3.48 €`
`($6.10 + £4.35) * 0.8 €` -> `8.36 €`
`$6.10 * (£0.2 + 0.8 €)` -> `£1.22 + 4.88 €`

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think folks will immediately want division (/) as well, can you add that ? We'll need to handle division by zero in some reasonable manner.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding it won't be hard at all, except for divide-by-zero; the operation parser wound up being amazingly extensible. I'm tempted to just make it silently fail, but it's probably better to work through the error reporting system to ensure sanity. I've not looked at that yet, though, so I might put division off until the end of the refactor, if you think that's all right.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

division can be a problem too... What would be the amount for $0.05 / 3 ?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.6666666666666666...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant: 0.016666666666666666...

### Virtual Postings

When you parenthesise the account name in a posting, we call that a
Expand Down Expand Up @@ -460,6 +492,28 @@ that no matter how many assertions you add, you can't be sure the
account does not contain some unexpected commodity. (We'll add support
for this kind of total balance assertion if there's demand.)


The asserted balance checks only the amount(s) of each listed commodity's
balance within the (possibly larger) account balance. We could call this a
partial balance assertion. This is compatible with Ledger, and makes it
possible to make assertions about accounts containing multiple commodities
without needing to manually track every commodity the account contains.

To instead assert a balance to the exclusion of all other commodities, use the
exact assertion form `==EXPECTEDBALANCE`. This ensures that the account does
ag-eitilt marked this conversation as resolved.
Show resolved Hide resolved
not contain some unexpected commodity, or, equivalently, that the balance of
any unlisted commodities is 0.

``` {.journal}
2013/1/1
a $1 = $1
b = $-1

2013/1/2
a 1€ = $1 + 1€
b -1€ = $-1 - 1€
```

#### Assertions and subaccounts

Balance assertions do not count the balance from subaccounts; they check
Expand Down Expand Up @@ -1242,9 +1296,14 @@ something.):
```

The posting amounts can be of the form `*N`, which means "the amount of
the matched transaction's first posting, multiplied by N". They can also
be ordinary fixed amounts. Fixed amounts with no commodity symbol will
be given the same commodity as the matched transaction's first posting.
the matching posting(s), multiplied by N". They can also be ordinary
fixed amounts (which are inserted unchanged) or [amount
expressions](#amount-expressions) headed by either (only the
multiplication to the left of the first addition or subtraction will be
applied to the posting value).

Any postings in real transactions which attempt to use this
multiplication-headed form will ignore the value instead.
Copy link
Owner

@simonmichael simonmichael Oct 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't understand "headed by" onward exactly. Shall we simplify again, deleting those lines ? Ie we will accept an amount, an amount expression, or a numeric multiplier (*N). (There'll probably be desire for \N, +N, -N, arbitrary arithmetic functions, referencing other parts of the transaction etc., but those can wait)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blame a couple years of linguistics. That's the passage that was primary in the "first draft" naming of the commit. I'll revisit this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the "those can wait", I might actually put the first step of the PR together without multiplication entirely. Addition and subtraction are a much more contained change, and seem to generate much less discussion (while being more frequently requested). We should definitely keep working out the questions around multiplication, but splitting things further like that means there's less pressure to resolve it so we can find something we're all happier with.


This example adds a corresponding ([unbalanced](#virtual-postings))
budget posting to every transaction involving the `expenses:gifts`
Expand Down