Skip to content

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
(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 ChickenProp force-pushed the insert-effective-date-region branch from bb47227 to 1afb772 Compare July 23, 2024 21:48
@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