ledger-xact: Add ledger-xact-fill to insert missing amount

[DamienCassou]

If the changes in this PR are desired, I will write unit tests.

[bcc32]

I think this command should consider whether the amounts in the postings are actually for the same commodity, e.g., if you had a transaction like:

2024-04-01 Hotel
    Expenses:Lodging    200 EUR
    Expenses:VAT
    Liabilities:Mastercard  -225 USD

It would not be correct to insert "25 EUR" or "25 USD" in the missing spot. (Also, it seems like this implementation does not insert a commodity symbol at all, is that intentional?)

[DamienCassou]

@bcc32 thank you for your quick feedback, I appreciate it. I think I have fixed my implementation to take commodities into account (I never use commodities and forgot about this use case, sorry).

[DamienCassou]

I changed the code to avoid a circular dependency between ledger-xact.el and ledger-post.el

[bcc32]

Hi @DamienCassou, I pushed a couple more commits to the branch. I think this fixes the commodity issue I was thinking of.

I'm a little unsure of what you think the correct behavior should be in this case:

2024-04-01 Hotel
    Expenses:Lodging                             225 USD
    Expenses:VAT
    Liabilities:Mastercard                      -225 USD

Currently the code will do nothing, but maybe it should insert an explicit zero amount? I tried to preserve as much of behavior in your original version as possible

[DamienCassou]

I'm so confused. You reviewed an old version of my work because I committed my changes to the wrong branch :-(. I'm sorry for your time.

The version I wanted you to review had support for commodity and avoided a circular dependency between ledger-post and ledger-xact. I think your work is better anyway so I will ignore mine. Here are the 2 functions I wrote just in case they prove useful in the future:

(defun ledger-xact-amounts ()
  "Return the amounts of the xact containing point or nil.

Each amount is of the form (VALUE COMMODITY), VALUE being a
number and COMMODITY a string."
  (save-excursion
    (cl-destructuring-bind (begin end) (ledger-navigate-find-xact-extents (point))
      (goto-char begin)
      (forward-line 1) ;; ignore the first line
      (let ((amounts))
        (while (< (point) end)
          (when-let* ((context (ledger-context-at-point))
                      ((eq (ledger-context-line-type context) 'acct-transaction))
                      (amount (ledger-split-commodity-string
                               (or
                                (ledger-context-field-value context 'commoditized-amount)
                                (ledger-context-field-value context 'amount)))))
            (setq amounts (cons amount amounts)))
          (forward-line 1))
        (nreverse amounts)))))

(defun ledger-xact-fill ()
  "Find a posting with no amount and insert it.

Even if ledger allows for one missing amount per transaction, you
might want to insert it anyway."
  (interactive)
  (cl-destructuring-bind (begin end) (ledger-navigate-find-xact-extents (point))
    (when-let* ((amounts (ledger-xact-amounts))
                (amounts-sum (cl-reduce #'ledger-add-commodity amounts))
                (missing-amount (ledger-subtract-commodity
                                 (list 0 (cadr amounts-sum))
                                 amounts-sum))
                (end-of-line-regex (rx (* (any space)) line-end)))
      (when (> (abs (car missing-amount)) 0.0001)
        (goto-char begin)
        (while (and (< (point) end)
                    (ledger-next-account end)
                    (match-end 0)
                    (progn
                      (goto-char (match-end 0))
                      (not (looking-at-p end-of-line-regex)))))
        (when (match-end 0)
          (insert "  " (ledger-commodity-to-string missing-amount))
          (ledger-post-align-dwim))))))

Please give me some more time to read your work, answer your question and maybe suggest some changes.

[DamienCassou]

@bcc32: I added some tests and strengthen yours by checking the content of the error message as well.

Regarding the case where an amount is missing but the transaction already balance, I decided to change the code to throw an exception and added a corresponding test case. My opinion is that the user should be aware there is something fishy instead of silently hiding the issue. If you disagree, I can implement what you have in mind instead.

If you agree with everything, feel free to squash all commits together and put your name there: there is not much code from me anymore anyway :-).

I'm sorry again for asking you to review the wrong version of my work. This definitively lead to more work for you.

[DamienCassou]

I found a bug, working on it.

[DamienCassou]

@bcc32 bug fixed. I had to refactor ledger-post-fill to avoid the duplication of the code searching for the missing amount. What do you think?

[DamienCassou]

I just removed the definition of 2 unused variables.

[bcc32]

I like the new approach overall, and thanks for the new tests. Added a couple of minor improvements to the branch directly, but I had a couple questions about some details (in the review interface).

[DamienCassou]

I answered your comments. What do you think is the best way forward?

[DamienCassou]

I took care of your feedback. Can you please have a look at the last commit? Thank you so much for your guidance and time.

[bcc32]

Thanks for your contribution, @DamienCassou! Looks great, merged.

[DamienCassou]

@bcc32 thank you so much for all your help. Most of the code is from you.