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

Amount expressions, multi-commodity postings #934

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ag-eitilt
Copy link
Contributor

This PR comes out of #913, which was closed as it both was only a small change (fixing before-unnoticed undefined behaviour) and was written in a way that was oriented more toward (this) future work than toward simply fixing that bug. Besides the larger scope here eclipsing that minor fix, some unclean commits and unfortunate naming resulted in the discussion there becoming confused; while the latter in itself would not be enough to close the earlier PR, I felt that taking it along with the former resulted in it making more sense to focus the conversation on the features introduced in later commits.

On that topic, this adds the ability to write postings usiing basic arithmatic operations inline, implementing #183 and #923, in a way that isn't hopelessly entangled with cruft, WIP code, and other features (as my first attempt #871 was). To separate out all its effects:

  • Posting amounts and assertations/assignments may be written as multiple amounts (whether of the same commodity or not) connected via addition or subtraction: $1 + $2
    • If multiple amounts of the same commodity are given, they are condensed into a single value: the above is equivalent to $3
    • If amounts of different commodities are given, they're equivalent to having put the individual amounts on separate lines, except that any transaction modifiers applied to that posting will only add static values once: = a ... *1 + $2 applied to a $3 + 4 CAD results in $3 + 4 CAD not $5 + 4 CAD
  • Commodities using parentheses now must be quoted: 1 "(XYZ)"
  • Standard postings are now no longer permitted to begin with a modifier-style multiplier

And, non-user-facing:

  • A Posting's amount is stored as a MixedAmount rather than a simple Amount
  • Amount no longer has the field amultiplier; its role is taken over by storing an Amount object in the new field Posting.pmultiplier
  • Hledger.Read.Common exports a new parser mamountp handling the amount expression logic
  • It also exports modifierp which functions mostly the same as amountp, except that its values are preceded by a * and never assigned the default commodity
  • The internal function multiplierp (previously only checking the presence of a single character) was rewritten to return the value of the (unitless) multiplier(s) and/or divisior(s) it matched
  • postingsp, postingp, and amountwithoutpricep now take a Bool argument indicating whether they're being parsed from within a TransactionModifier

@ag-eitilt
Copy link
Contributor Author

The Travis failiure isn't in any of my code, so I'm not entirely sure why it's only appeared for this push. Maybe one of the dependencies updated in a fragile way?

@simonmichael
Copy link
Owner

Thanks for the PR.

Interesting travis failure.. it appears to building filemanip, and failing. It might or might not have anything to do with commercialhaskell/stackage#4206 . hledger is supposed to be using Glob, not filemanip, I don't know if something else is depending on filemanip.

@simonmichael simonmichael added A-WISH Some kind of improvement request, hare-brained proposal, or plea. journal The journal file format, and its features. help wanted needs:value-proposition To unblock: needs clearer justification, review of benefits vs costs needs:impact-analysis To unblock: needs analysis of interactions with other features, users, ecosystem needs:review To unblock: needs more code/docs/design review by someone labels Dec 8, 2018
site/doc/1.12/journal.md Outdated Show resolved Hide resolved

Note, however, that any commodity symbols must be written alongside the amounts
they describe, not split by parentheses: `$(1 + 2)` can *not* be read.

Copy link
Owner

Choose a reason for hiding this comment

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

Terrific docs!

- either of the three previous modifiers, followed by a plus or minus sign
and a normal amount or amount expression, eg `*2 + $1`. The matched
posting's amount will be multiplied or divided as before, and the rest of
the expression will be added to the result.
Copy link
Owner

Choose a reason for hiding this comment

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

This feature feels like an unnecessary complication at first glance - perhaps first PR should focus just on adding amount expressions, which is a big change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see where you're coming from, but I'm not sure whether people will find it more confusing to have amount expressions in transaction modifiers except when they're multipliers, or to have transaction modifiers doing two things at once. The latter felt a bit more natural to me, obviously, but thinking about it I do tend to have a more multi-track mind than most (though with most users being people on Linux using the console...) It's not a functional difference from that side since even if we do split things, users can just put the multiplier and the expression on separate lines.

Basically, I see more people getting confused if they're able to write ($1 + $2) * 3 + $4 but not apply x * 3 + $4 to ($1 + $2) than I see people getting thrown off by both x * 3 and y + $4 being on one line.

From the technical side, this is essentially just a user-facing interface to the split between pamount and pmultiplier; I'm not sure we really want to merge the two into an Either since we'd then need to handle all the corner cases (in printing, say) where a posting has a Quantity instead of an Amount. Without multipliers + expressions in journals, though, there's not as much impetus keeping them apart, and no way of using shelltest to test the behaviour when both are present.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to clarify, that's not shutting down my going back and not putting that in, it's just my argument for why I think it belongs. If you do think it should go, than I'm happy enough cutting it.

@ag-eitilt
Copy link
Contributor Author

Actual test failures this time! Strangely, though, they're not showing up with my local copy. I'll take a look at that when I get the chance.

@simonmichael
Copy link
Owner

Sorry for the lack of input.. it may take me a little longer to get back to this.

@simonmichael simonmichael added the needs:rebase To unblock: needs to be rebased against latest master branch label Jan 5, 2019
@simonmichael
Copy link
Owner

simonmichael commented Jan 5, 2019

Part of the reason for delay: I already feel hledger's complexity is too hard to keep up with, and I can't assume any serious increase in our maintenance capacity, so I fear the complexity and cost this PR may bring and I'm almost anti-motivated to merge it, especially as I don't yet see or feel any pressing need for it.

So, possible next steps:

  • rebase it against master - I have fixed the failing roi tests
  • I and hopefully others will install and play with it. Often that will change my mood.
  • let's spend some time using and refining it - as your fork if necessary - and gather a bunch real world examples where it's useful

@simonmichael simonmichael removed the needs:review To unblock: needs more code/docs/design review by someone label Jan 5, 2019
@simonmichael simonmichael changed the title Amount expressions Amount expressions, multi-commodity postings Jan 7, 2019
@simonmichael
Copy link
Owner

Hi @ag-eitilt , will you have time to rebase this against latest master any time soon ?

@simonmichael
Copy link
Owner

(If not, let me know and I might try.)

@simonmichael simonmichael removed the needs:value-proposition To unblock: needs clearer justification, review of benefits vs costs label Mar 15, 2019
@simonmichael simonmichael added the needs:testing To unblock: needs more developer testing or general usage label Jul 15, 2019
@lestephane
Copy link

lestephane commented Nov 7, 2019

I may have a use case, but I'm not sure this PR supports it. If you think it does,
I can try it and report:

~ 2018  residency rules
    Cyprus      60 days ; minimum days to spend in Cyprus
    Germany     90 days ; maximum days to spend in Germany 
    France      MAX(Cyprus,Germany) - 1 days ; maximum days to spend in France

Note that the France rule refers to the current value of Cyprus and Germany, not the value from the automated rules above it. I think my use case is too contrived, but I thought I might as well mention it, since I put more and more in hledger these days

@ShrykeWindgrace
Copy link

My usecase for this feature is the semi-automated splitting of expenses, something to support the transactions of the form

2019/12/12 Restaurant
    Assets    -100
    Expenses   100/2
    Liabilities:SomeOtherPerson  100/2

The support for fractional (not necessarily decimal) amounts would be great, too. Another recent usecase which lead me to finding this PR, is "three people chip in to buy two gifts; the total sum is divisible by three, but individual prices are not divisible by three". Either I lose granularity of transactions I want and dump everything in one posting (don't really want to), or deal with rounding errors that I need to manually fix later (which leads to ugly postings and balances).

@l29ah
Copy link

l29ah commented Nov 6, 2023

I am trying to bring this PR up to date with the current master branch and got confused during rebasing due to some of the work being duplicated in master, e.g. 7ed2a0a
What is the purpose of allowCommodityMult argument in postingp?

@simonmichael
Copy link
Owner

I can't find any such name

@simonmichael
Copy link
Owner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-WISH Some kind of improvement request, hare-brained proposal, or plea. journal The journal file format, and its features. needs:impact-analysis To unblock: needs analysis of interactions with other features, users, ecosystem needs:rebase To unblock: needs to be rebased against latest master branch needs:testing To unblock: needs more developer testing or general usage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants