Separate multiplier from Amount

[ag-eitilt]

In one of the several discussions in #871, @simonmichael said:

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.

This then is that enforcement. However, as there are indeed some tests relying on the old behaviour (misc/rewrite.test:3 and :4), we likely need input from others on whether this is the direction to go ---in which case I'll finish the polish of this, including docs--- or whether we do want to allow changing the commodity ---and whether we want to make the syntax more explicit so information isn't discarded accidently.

[simonmichael]

@ag-eitilt now I see what you were saying: auto-generated postings do allow replacing the matched amount's commodity, as seen in rewrite.test. I've added more tests in tests/print/auto.tests describing this (f583301).

I agree with you that this can be useful in auto posting rules, and we should still allow it there for now (and ensure adequate docs). This (prefix) star in auto posting rules is different from the (infix) star in the proposed new amount expressions, currently at least.

I still think the latter (amount expressions) should not allow mixed-commodity multiplication/division, but I could reconsider that if we find compelling use cases.

[simonmichael]

PS if you work more on this PR, let's replace the commits with a new one based on latest master.

[simonmichael]

Checking in. IIRC the status of this is needs changes, ie we want to disallow overriding the left hand commodity in general amount expressions, but not in transaction modifier posting amount rules.

[simonmichael]

Also:

• needs rebase against master
• this seems to be just a small refactoring step which I think needs a little more motivation, we could enlarge this PR a bit (eg add the amount expressions), or start a new one if easier.

[sorsasampo]

@simonmichael I think it would be nice if there was a separate issue for expression amounts that people could subscribe to.

I followed #183 / #923 -> #871 comments here.

[simonmichael]

@sorsampo: reopened #183

[ag-eitilt]

Finally got back to this; sorry it took so long! Anyway, as you noted, the previous status was simply more or less a request for comment (and should probably have been named as such). This now is where I'd consider it motivated; for the PR reviewers, the major change is:

• Prevent standard transactions from using the transaction modifier multiplier syntax: (b) *2

In service of that, it also introduces the minor changes of:

• Amount no longer has the field amultiplier; instead, that role is moved to Posting as the new field pmultiplier
• multiplierp (previously internal to Hledger.Read.Common, simply checking the next character was *) now instead reads a complete Amount preceded by a *, and is exported
• Some parse functions internal to their modules (amountwithoutpricep, postingsp, and postingp) have an additional Bool parameter indicating whether they've been called as part of a transaction modifier/posting multiplier

This hasn't yet been added to the docs; for now, your TODO in Hledger.Data.TransactionModifier should still work as a reminder until one or the other of us (probably me) gets around to writing it up.

[simonmichael]

Thanks! Please add the docs changes when you get the chance.

[simonmichael]

"Restore ability..." - makes me wonder when they lost that ability. Was it in the previous commit, & so should this be merged with that ? Or was it an older one we already merged ?

[ag-eitilt]

It was the previous commit, and it's not really even required to merge the two; that one was moving things in a direction that we don't actually need until I add multiplication, and there's better ways of doing so now. I'll remove it. By the way, for my part I do see version control as being more for tracking development history than for a project breakdown. Definitely, all the code checked in should be buildable and function as nicely as possible, but other than that minimum, I feel there's merit in keeping code that's ultimately reverted. I know that's not your mentality and I'm happy enough to keep the logs clean when I'm working here because of that, but I just wanted to let you know why I keep giving you messy commits: I'm used to keeping them in when I'm in my own repos.

…

On November 20, 2018 1:10:54 AM UTC, Simon Michael ***@***.***> wrote: simonmichael commented on this pull request. > @@ -47,7 +47,7 @@ import Hledger.Utils.Debug -- 0000/01/01 "Restore ability..." - makes me wonder when they lost that ability. Was it in the previous commit, & so should this be merged with that ? Or was it an older one we already merged ? -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #913 (review)

[simonmichael]

Sorry @ag-eitilt, but I'm confused. Is this PR intended to be merged ? I thought it was.

[simonmichael]

I've reread the original description, but I don't remember enough context to be clear. You mentioned doc changes - if those are appropriate, please add them to the PR, that will help me understand.

[simonmichael]

And now I've reread the new description. This makes it sound like (and the the commit looks) like an internal refactoring that shouldn't affect anything users have been doing up to this point. Is that accurate ?

[simonmichael]

Old way: the amultiplier boolean flag turns a standard Amount into something else (a transaction modifier posting's amount-or-amount-multiplier). New way: the pmultipler amount turns a standard posting into something else (a transaction modifier posting with amount or amount multiplier).

I agree the old way is inelegant. Is the new way better ? It seems to me "even more wrong" semantically. Eg now you could have a Posting with both a multipler amount and a regular amount, which has no connection to the journal syntax.

[ag-eitilt]

Yeah, sorry. *That commit* you were asking about superceeded the previous (also in the PR, before the latest forced push) for all but one or two lines, so I flattened them to remove the changes introduced then immediately removed in favor of others. To make it confusing, it's using the description of the earlier (changes dropped) but the code of the later (description dropped). Basically, this is still a viable PR, and can be merged (pending review) as soon as I add the docs in the next day or two, and fix anything else that comes up. As for what it's doing, it's only a small change and *shouldn't* affect reasonable users, but it *does* fix the bug where a multiplier amount (like for transaction modifiers) could be entered in standard transactions, with undefined behaviour (probably just treating the multiplier as a value -- `* 0.5` being silently converted to `$0.50` or whatever the journal's default commodity is -- but with the potential for weirder behaviour down the line).

…

On November 20, 2018 4:04:10 PM UTC, Simon Michael ***@***.***> wrote: Sorry @ag-eitilt, but I'm confused. Is this PR intended to be merged ? I thought it was. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #913 (comment)

[simonmichael]

I see that

2018/1/1
  (a)   *$1

is parsed successfully by hledger (eg 1.10, 1.11) as you say. The star seems to be just ignored, and it doesn't seem like a bug anybody would have hit. Good catch, but it probably isn't something that needs to be mentioned in docs. (Release notes maybe).

[ag-eitilt]

Heading to work now, and will respond once I get back, but good catch, and I might close this PR as being more foundation for another.

…

On November 20, 2018 4:21:21 PM UTC, Simon Michael ***@***.***> wrote: simonmichael commented on this pull request. > @@ -256,6 +254,7 @@ data Posting = Posting { pcomment :: Text, -- ^ this posting's comment lines, as a single non-indented multi-line string ptype :: PostingType, ptags :: [Tag], -- ^ tag names and values, extracted from the comment + pmultiplier :: Maybe Amount, -- ^ optional: the proportion of the base value to use in a 'TransactionModifier' Old way: the amultiplier boolean flag turns a standard Amount into something else (a transaction modifier posting's amount-or-amount-multiplier). New way: the pmultipler amount turns a standard posting into something else (a transaction modifier posting with amount or amount multiplier). I agree the old way is inelegant. Is the new way better ? It seems to me "even more wrong" semantically. Eg now you could have a Posting with both a multipler amount and a regular amount, which has no connection to the journal syntax. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #913 (review)

[simonmichael]

Ok thanks Samuel May. I will be travelling, so no urgency on my end.

[ag-eitilt]

Closed in favor of #934.