Closed SqrtMinusOne closed 5 months ago
I can't figure out how this works. From what I can gleam from the source code, the other parts of the code that use org-extend-today-until
seem to take a different approach: https://github.com/bastibe/org-journal/blob/master/org-journal.el#L802
Could you comment on why this works? time-since
seems to return a time-delta, not a point-in-time, but org-journal--get-entry-path
appears to require a point-in-time?
(time-since arg)
is (time-subtract nil arg)
for non-string values of ARG
. time-subtract
takes two time values and returns a time value, where nil
is the same as (current-time)
.
An integer is a possible time value, see (elisp) Time of Day. So, (time-subtract nil number-of-seconds)
moves the time value by number-of-seconds
to the past.
E.g. (time-subtract nil 0)
is the current time, see
(format-time-string "%FT%T%z" (time-subtract nil 0))
Or, to make it a year ago:
(format-time-string "%FT%T%z" (time-subtract nil (* 60 60 24 365)))
Thus (time-since (* 3600 org-extend-today-until))
is the time value org-extend-today-until
hours ago.
As I said, this is how org-today
works, see https://github.com/bzg/org-mode/blob/098f0815916fcfd88b24d7a0a842c3b294b4383d/lisp/org.el#L5061.
time-to-days
also accepts a time value ((elisp) Time Calculations). Ironically, days-to-time
is not the inverse of time-to-days
, so I decided not to reuse org-today
.
As for your link, I've changed the code as follows and it seems to work the same:
(let* ((time (time-since (* 3600 org-extend-today-until)))
(org-extend-today-until-active-p
(> (time-to-days nil)
(time-to-days time)))
...)
...)
I can commit that as well.
I see! Thank you very much for the clarification, this is clever!
I think I would prefer (time-subtract (current-time) (* 3600 org-extend-today-until))
, as it makes the intent clearer (moving the time). It seems to me that the intent of time-since
is instead for measuring a time difference, but maybe that's just a misconception on my part.
Maybe you're right, changed that.
As for me, I often look at the source when I see a function I don't understand, and the nature of time-since
is immediately clear from doing that. But perhaps I wrote too much elisp.
As for me, I often look at the source when I see a function I don't understand
I tend to look at the documentation only, which is probably where my confusion comes from. But I'll also freely admit that my elisp is quite rusty these days.
The Pull request looks good to me. Is it ready to merge from your end?
Is it ready to merge from your end?
It is.
The errors in CI look like they weren't introduced by me.
Thank you @SqrtMinusOne
Yes, I had to invoke that function at 3 AM on Monday to discover that bug. That's unfortunate. That must have been very "annoying".
I see a couple of other things we could improve as well.
org-extend-today-until
is used, a re-factoring might be the better solution in the long run
https://github.com/bastibe/org-journal/blob/129dede1a0c61e0fd914ed9b9aed2baab831ee7c/org-journal.el#L805
https://github.com/bastibe/org-journal/blob/129dede1a0c61e0fd914ed9b9aed2baab831ee7c/org-journal.el#L1271
https://github.com/bastibe/org-journal/blob/129dede1a0c61e0fd914ed9b9aed2baab831ee7c/org-journal.el#L1619Anyway, it would be perfectly fine to merge it, and create a new issue to resolve one or the other point a little :-) later.
Ah, regarding the CI, I know about it, I know how to fix it quickly, but I don't like my "dirty hotfix"🙈
I can do 1 and 3 in this PR a bit later, no big deal.
As for 2 - I can write it as (* 60 60)
, but I feel like it's immediately obvious that 3600 is the number of seconds in an hour, especially in the context of time manipulation :slightly_smiling_face:
Org Mode devs do not seem to have a problem with hardcoding it, although I admit that core Emacs and ELPA often have lower code quality than what's required by MELPA. Hyperbole is still not admitted to MELPA...
But anyway, I really don't like something such as this:
(defconst org-journal--seconds-in-hour (* 60 60)
"The number of seconds in an hour.")
Perhaps a (let ((seconds-per-hour 3600)))
would do the trick? Or simply add a comment? Personally, I find 3600
acceptable, as it is clear from context that this converts hours to seconds. But if it's confusing to others, it's probably worth being explicit.
As for 2 - I can write it as
(* 60 60)
, but I feel like it's immediately obvious that 3600 is the number of seconds in an hour, especially in the context of time manipulation 🙂
Never mind. But we should ensure that org-extend-today-until
works as expected in all the locations it is used, aka write more tests. I'm currently working on moving to Eask.
I'll create a new task for the other two points (1 and 3).
This makes
org-journal-open-current-file
respectorg-extend-today-until
. The variable is used the same way as inorg-today
.Yes, I had to invoke that function at 3 AM on Monday to discover that bug.