-
Notifications
You must be signed in to change notification settings - Fork 75
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
base: master
Are you sure you want to change the base?
Allow ledger-insert-effective-date
to work with regions.
#404
Conversation
There was a problem hiding this 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) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
bb47227
to
1afb772
Compare
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 |
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.