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

Provide functions for shifting date on the current line #47

Merged
merged 1 commit into from
May 20, 2024

Conversation

vkazanov
Copy link
Contributor

Thank you for the amazing mode! It has almost everything I need to work with .beancount files.

One thing I was missing from org-mode is an ability to quickly shift transaction dates by date. The functions suggested (complete with unit tests) provide the functionality.

Happy to tweak a bit if maintainers feel that something is missing.

@dnicolodi
Copy link
Collaborator

Thanks, it looks like a nice addition. However, I don't think emitting an error if the current line does not contain a date is a good idea. I'm also unsure about picking any date on the current line. I think it would be better to only operate on a date under the current point.

@vkazanov
Copy link
Contributor Author

Hi @dnicolodi ,

On the error. I thought about silently ignoring the situation but the manual provides the following example:

For example, if you try to use the command Info-history-back (l) to move back beyond the start of your Info browsing history, Emacs signals a user-error

In general, this is the standard way to signal function use outside of the valid context (i.e. user-side error).

Let me know if you still think user-error is not the right way to do it there. I am fine with both but signalling something might make sense.

On only making this work within the timestamp itself. Not a problem, will do.

Thanks for looking into this!

@vkazanov
Copy link
Contributor Author

@dnicolodi i've fixed the function to only use dates under point.

Easy to change the user-error as well but let me know if you still think it makes sense. Thanks!

beancount.el Outdated
(save-excursion
(beginning-of-line)
(when (re-search-forward date-regexp (line-end-position) t)
(replace-match newdate t t nil)))))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this can be simplified to

(defun beancount--shift-date-at-point (days)
  "Shift the date under point by a specified number of DAYS."
  (save-excursion
    (if (thing-at-point-looking-at beancount-date-regexp 10)
      (let ((newdate (beancount--shift-date (save-match-data (date-to-time (match-string-no-properties 0))) days)))
        (replace-match newdate t t))
      (user-error "No date at point"))))

This gets simpler with the changes to beancount--shift-date proposed above, so much that the let may not be helping readability that much anymore.

With this simplification, I'm not sure that (save-excursion ...) is needed.

beancount.el Outdated
DAYS can negative or positive."
(format-time-string
"%Y-%m-%d"
(time-add today (days-to-time days))))
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's kind of weird to have a function accept Emacs-epoch days but return a formatted string. I would prefer this function to take a formatted date or nil and return a formatted date. As the date parsing functions (surprisingly) clobber the match data, it should also use (save-match-data ...) around date-to-time. Bonus points for replacing date-to-time with some function more accurate for the task from iso8601.el.

@dnicolodi
Copy link
Collaborator

The tests are failing with older Emacs releases, anything before 29.1. I would prefer to do not have to bump the required Emacs version just for this. Can you please check which function has been introduced or has changed his behavior in Emacs 29?

@vkazanov
Copy link
Contributor Author

@dnicolodi ,

I've integrated the suggestions and made sure the code works with Emacs 27, 28, 29. I don't have to access to older versions.

The date parsing code in the patch, while not ideal (save-match-data is still there), does work across 3 recent Emacs versions without introducing version-dependant branches into the function. I couldn't come up with a better variant as date parsing expose subtly diffent behaviour across versions.

Let me know if something else needs to be handled.

@vkazanov vkazanov requested a review from dnicolodi May 20, 2024 10:27
@vkazanov
Copy link
Contributor Author

Hm. So 26 doesn't work.

Is Emacs version 26 the lowest common denomator targeted by beancount-mode?

@dnicolodi dnicolodi force-pushed the main branch 9 times, most recently from 9787239 to 79245d7 Compare May 20, 2024 18:14
@dnicolodi
Copy link
Collaborator

dnicolodi commented May 20, 2024

I simplified the code slightly, fixed it to work on Emacs 26, and extended it a tiny bit to make sure that the point does not jump around when a date is incremented or decremented. PTAL. I write Emacs lisp so seldom that every time it is a pain to get into it again... I took a few iterations but I made it work...

@vkazanov
Copy link
Contributor Author

Thanks! All reasonable changes.

But are you sure emacs 26.1 is still worth it? This is when core development started gaining steam, with many nice things added in 27 and onwards

@dnicolodi
Copy link
Collaborator

If all it takes to keep beancount-mode compatible with Emacs 26 is a three lines macro, I don't see reason to drop support for it. Beancount itself still supports Python 3.7, thus it makes sense to support old Emacs releases too. On the other hand, Eamacs 27 was released in August 2020, thus if more reasons to drop Emacs 26 support pop up, we can definitely do it.

@dnicolodi
Copy link
Collaborator

This project sees an average of two commits per year, thus backward compatibility has not been a major issue so far 😃

@dnicolodi dnicolodi merged commit 8cafe1c into beancount:main May 20, 2024
10 checks passed
@vkazanov
Copy link
Contributor Author

vkazanov commented May 20, 2024

Makes sense :-)

Beancount is a simple format, with a self-contained file. The mode doesn't need too many features.

I might add a commit or two per year myself - began using plain text accounting recently, and it's a blast!

Also, have you ever using eask for development? Simplifies managing multiple emacs versions, and testing in general.

@dnicolodi
Copy link
Collaborator

I've never heard of Eask before, but I'm not much of an Emacs Lisp developer.

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

Successfully merging this pull request may close these issues.

2 participants