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

Relative dates in deadlines and scheluded for before and after #149

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dancn
Copy link

@dancn dancn commented Apr 24, 2020

Backward compatible improvement, org-read-date support a super-set of
the previous function org-time-string-to-absolute.

Should be also an easy solution for #59.

@dancn dancn changed the title Relative dates in dedalines and scheluded for before and after Relative dates in deadlines and scheluded for before and after Apr 24, 2020
@alphapapa alphapapa self-assigned this May 1, 2020
@alphapapa
Copy link
Owner

Thanks, this looks like a nice improvement. I will look at it more closely when I have some time, hopefully soon.

@alphapapa alphapapa added this to the 1.3 milestone May 1, 2020
@nivekuil
Copy link

+1, I use this patch locally and it's quite convenient.

@pkazmier
Copy link
Contributor

pkazmier commented Dec 9, 2020

Yes, I, too, think this would be incredibly useful, so much so, I submitted the exact same PR without realizing this one existed!

@dancn
Copy link
Author

dancn commented Apr 26, 2021

After one year I re-based the branch for my convenience, hopefully will also speed up the review and merge.

@real-or-random
Copy link

It would be great to see this merged!

@alphapapa
Copy link
Owner

Thanks for the ping and your patience.

Copy link
Owner

@alphapapa alphapapa left a comment

Choose a reason for hiding this comment

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

Thanks for your patience. I think this shouldn't need much more before it can be merged.

Also, would you add tests for this, please? (i.e. in the test suite, not just that Org file)

@@ -191,7 +191,7 @@ These selectors take one argument alone, or multiple arguments in a list.
+ =:category= :: Group items that match any of the given categories. Argument may be a string or list of strings.
+ =:children= :: Select any item that has child entries. Argument may be ~t~ to match if it has any children, ~nil~ to match if it has no children, ~todo~ to match if it has children with any to-do keywords, or a string to match if it has children with certain to-do keywords. You might use this to select items that are project top-level headings. Be aware that this may be very slow in non-daily/weekly agenda views because of its recursive nature.
+ =:date= :: Group items that have a date associated. Argument can be =t= to match items with any date, =nil= to match items without a date, or =today= to match items with today’s date. The =ts-date= text-property is matched against.
Copy link
Owner

Choose a reason for hiding this comment

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

Should other selectors, like :date, also support this?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your patience. I think this shouldn't need much more before it can be merged.

Thanks for the review!

Also, would you add tests for this, please? (i.e. in the test suite, not just that Org file)

Just added a quick test (not pushed yet). It fails, I guess that org-read-date is not aware of org-super-agenda-test-date so relative date specification can not be tested without further mocks. Do you have some suggestion on how to do that?

:date does not support arbitrary dates yet (only static: any, none, today), so relative ones are a double step!

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for your patience. I think this shouldn't need much more before it can be merged.

Thanks for the review!

FYI, feel free to ping me more often in the future. I didn't intend to ignore this for a year; I just hadn't thought about working on org-super-agenda for a while, since I didn't need to change anything in it for my own sake, and I have a number of other projects that occupy my free time.

Also, would you add tests for this, please? (i.e. in the test suite, not just that Org file)

Just added a quick test (not pushed yet). It fails, I guess that org-read-date is not aware of org-super-agenda-test-date so relative date specification can not be tested without further mocks. Do you have some suggestion on how to do that?

I guess you'll have to look in the Org source code, find the appropriate function, and redefine it locally in the test using cl-letf.

:date does not support arbitrary dates yet (only static: any, none, today), so relative ones are a double step!

As you can tell, I haven't worked on this package for a while. :) We should probably fix that too, though maybe not in this PR.

`org-time-string-to-absolute' can process."
DATE', where DATE is a date string that `org-read-date' can
process. Note that relative dates are supported, e.g. `before
+3d' means in the next two days."
Copy link
Owner

Choose a reason for hiding this comment

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

The +3d here should be in a string, right?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, do you think that is not clear?

Copy link
Owner

Choose a reason for hiding this comment

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

The part in the backtick-apostrophe quote indicates Lisp code, i.e. like (:scheduled (before "+3d")). Without the string, it wouldn't be valid, e.g. (:scheduled (before +3d)). So the +3d should appear as a string, I think.

`org-time-string-to-absolute' can process."
DATE', where DATE is a date string that `org-read-date' can
process. Note that relative dates are supported, e.g. `before
+3d' means in the next two days."
Copy link
Owner

Choose a reason for hiding this comment

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

See previous comment. :)

@dancn
Copy link
Author

dancn commented Oct 11, 2021

Hello,
I tried to add some tests with the last push, but failed. Please comment/suggest on the new tentative. Thanks

@alphapapa
Copy link
Owner

It shouldn't be necessary to use advice for this. See

(cl-defmacro org-super-agenda-test--with-mock-functions (fns &rest body)

Also, FYI, the advice you wrote uses the old-style advice. When writing advice, you should instead use the new-style nadvice library that uses add-function and advice-add. See the Elisp manual.

Backward compatible improvement, org-read-date support a super-set of
the previous function org-time-string-to-absolute.

The tests are working, but hugly hacks to force org-read-date to compute
a relative date to a fixed date.
@dancn
Copy link
Author

dancn commented Oct 15, 2021

Another try here! I found out out to get relative date to a fixed date... the test code is not very lispy but works and covers the obvious cases.

@dancn
Copy link
Author

dancn commented Oct 28, 2021

@alphapapa or anybody interested, please have a look at the patch and request changes or improve it! Thanks!

@eugr
Copy link

eugr commented Jan 24, 2022

Any updates on this? Anything preventing this from being merged? This would be a mega-useful feature as there is no good workaround for this currently.

@DominikMendel
Copy link

I did this as a local work around to adjust for the number of days. Hope it helps anyone else. It's a little janky.

(defun my/get-time-string-today-offset (dayOffSet)
  (format-time-string "%Y-%m-%d %H:%M" (+ (string-to-number (format-time-string "%s" (current-time))) (* 86400 dayOffSet))))

And call it like this:

  (let ((date (my/get-time-string-today-offset 5)))
....
`( ...
:scheduled (after ,date)
)

@alphapapa
Copy link
Owner

Any updates on this? Anything preventing this from being merged? This would be a mega-useful feature as there is no good workaround for this currently.

@eugr What prevents it from being merged is chiefly my lack of time to work on it. I don't have as much to spend on this project as I used to.

Besides that, there are issues of code quality, tests to be reviewed, etc. that must be addressed before it can be merged. Some of them I have mentioned to the PR's author in comments, so in that respect, the PR is waiting on him to address them, and then on me to re-review and confirm that it's appropriate for merging.

#225 is also a possibility for merging, but it also needs to be polished and re-reviewed, and its author has not responded to my last comment.

Of course, if either of these PRs works for you, you're free to use their code as-is right now.

@alphapapa alphapapa removed this from the 1.3 milestone Sep 23, 2023
@alphapapa alphapapa added this to the 1.4 milestone Sep 23, 2023
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.

7 participants