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

Allow ledger-insert-effective-date to work with regions. #404

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

Conversation

ChickenProp
Copy link

It updates or removes the effective date of all transactions that start in the region (technically, that start on a line in the region), without editing any postings.

With my credit card, I use the statement date as the effective date. That means I have a long block of transactions that all have the same effective date. This makes it easy to set them all at once.

@bcc32 bcc32 self-assigned this Jul 14, 2024
Copy link
Collaborator

@bcc32 bcc32 left a comment

Choose a reason for hiding this comment

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

Hi, thanks for your contribution!

In addition to the inline comments I have left, can you also please rebase your branch and add some tests for the new functionality? I have just pushed some baseline tests for the existing ledger-insert-effective-date to the master branch.

ledger-mode.el Outdated
@@ -176,34 +176,75 @@ the balance into that."
nil 'noerr)
(replace-match ""))))))))

(defun ledger-insert-effective-date (&optional date)
(defun ledger-insert-effective-date (&optional start end date)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding optional arguments before other optional arguments is a breaking change to the Lisp interface; please add these at the end of the arguments list, instead.

(ledger-remove-effective-date)
(insert " ; [=" date-string "]"))))))))

(defun ledger-insert-effective-date-region (start end &optional date)
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's a lot of duplicated code between this function and ledger-insert-effective-date. Can they be combined? I would think that just looping over the lines in start..end and calling ledger-insert-effective-date (with the correct current-prefix-arg) should be sufficient.

Copy link
Author

Choose a reason for hiding this comment

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

I managed to reduce it a bit, but there's still a decent amount of duplication.

It updates or removes the effective date of all transactions that start
in the region (technically, that start on a line in the region),
without editing any postings.

With my credit card, I use the statement date as the effective date.
That means I have a long block of transactions that all have the same
effective date. This makes it easy to set them all at once.
Need to keep `date` first for backwards compatibility. Fixes tests.
@ChickenProp
Copy link
Author

Tests added. They're not as thorough as I'd like: in particular, I don't know how to test prompting behavior, and when refactoring I briefly introduced a bug that my tests didn't catch. (If I called (ledger-insert-effective-date date) instead of (... date-string), then interactive use would ask for a different date for every xact in the region, plus one up front that would be ignored. Tests don't catch it because they never get dates from prompts.)

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

Successfully merging this pull request may close these issues.

2 participants