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

Timestamp ranges not matched the same way org-agenda does #237

Merged
merged 4 commits into from
Jun 26, 2024

Conversation

yantar92
Copy link
Contributor

@yantar92 yantar92 commented Aug 28, 2021

I have a headline like the following:

***** NEXT [#A] Manuel Macías [Email] (2021) "Continuous Integration and TeX with Org-Mode", by Rohit Goswani (TUG 2021) :BOOKMARK:misc:email:
SCHEDULED: <2021-08-07 Sat 16:30-17:30>

I expect this headline to be matched by (scheduled 0) sexp, but it is not.

@alphapapa
Copy link
Owner

I may be mistaken, but I don't think it's valid to use a range timestamp as an entry's SCHEDULED timestamp.

@yantar92
Copy link
Contributor Author

yantar92 commented Aug 7, 2021 via email

@alphapapa
Copy link
Owner

I may be mistaken, but I don't think it's valid to use a range timestamp as an entry's SCHEDULED timestamp.

Well, I guess we'll have to do it too, then. :)

It is indeed discouraged in the manual. However, org-agenda does show scheduled entries with scheduled timestamp being a time interval. Also, ts predicate will not match timestamps with interval (see `org-ql-regexp-part-ts-time').

Hm, I thought I had that covered, but maybe I overlooked it. Thanks.

@alphapapa alphapapa self-assigned this Aug 10, 2021
@alphapapa alphapapa added the bug label Aug 10, 2021
@alphapapa alphapapa added this to the 0.6 milestone Aug 10, 2021
@alphapapa alphapapa modified the milestones: 0.6, 0.7 Sep 22, 2021
@alphapapa
Copy link
Owner

Retargeting this for 0.7. 0.6 has been delayed for too long.

@stfl
Copy link

stfl commented Dec 8, 2021

It's also relevant for ts-active timestamps. In particular this shows up for me when syncing my calendar with org-gcal which will create active ranged timestamps.

@alphapapa alphapapa changed the title scheduled predicate ignores headings scheduled to a time range Timestamp ranges not matched the same way org-agenda does Mar 1, 2022
@alphapapa alphapapa force-pushed the master branch 2 times, most recently from 4f5fbc4 to d0acc8c Compare May 30, 2022 16:27
@enko
Copy link

enko commented Aug 9, 2022

Anything hindering this pr to be merged?

balaramadurai added a commit to balaramadurai/org-ql that referenced this pull request Aug 14, 2022
scheduled time-range now covered

this is the code from alphapapa#237
@alphapapa
Copy link
Owner

@enko Well, yes: since this has been a bug, the fix should be accompanied by explicit tests, i.e. there should be a new test added for timestamp ranges, rather than just changing one of the test data entries, as the PR currently does. Maybe even multiple tests for active/inactive timestamps, in planning/deadline/scheduled lines, and in headings/content.

It also needs a changelog entry--which I typically take care of myself when merging, but if a contributor provides it in the patch, it saves me the time and makes it easier for me to merge (assuming it's done correctly; even when I ask someone to provide one, and they do, it usually contains typographical errors that I have to fix myself, which defeats the purpose).

Other than that, the limitation is time, which I don't have as much of as I used to.

In the meantime, you're free to use the PR's code and report back as to how well it works. Any bugs you could discover before merging would be helpful.

@alphapapa
Copy link
Owner

As well, fixing this properly probably requires more work, because a ranged timestamp essentially represents two timestamps, one at each end of the range, and searching for timestamps within specific ranges must account for both of them.

@dmitrym0
Copy link

Do you think it's possible to split this is up into two tasks:

  1. getting ranged timestamp to match scheduled and planning
  2. searching for a timestamp in a range.

I came here look for 1, looks like I'm not the only one. Would be happy to contribute tests for it as well and improve the original request.

@magogware
Copy link

I tried out your most recent commit @dmitrym0 and had no luck with either the scheduled predicate or the ts-active predicate.

@dmitrym0
Copy link

@magogware

Here's a part of my test data:

* TODO This heading should be there because it contains scheduling info with time
SCHEDULED: <2026-01-23 Sun 14:00-15:00>
:PROPERTIES:
:ID:       FAKE_ID1
:END:

and the associated org-ql query that returns it:

'(or
  (ts-active)
  (clocked)
  (and
   (todo "TODO" "DONE")
   (or
    (scheduled) (planning))))

@magogware
Copy link

Ack, sorry. A (Doom) Emacs restart ensured I was actually using the package built from your commit, and now everything works. Cheers! It all works a charm.

@dmitrym0
Copy link

Can't take credit for it; so thanks to @yantar92. Of course caveat emptor since this hasn't been properly tested.

@alphapapa
Copy link
Owner

alphapapa commented Nov 1, 2022

@yantar92 FYI, it appears that something in the Buttercup library has changed, and now the test suite goes into an infinite loop and never terminates. Obviously, this presents a bit of a roadblock to developing this and other fixes and features. And my time and enthusiasm for debugging a third-party library that used to work fine is limited.

I've half a mind to convert all the tests to use ERT, but I like Buttercup's nice-looking, grouped and colorized output. Maybe what's needed is some kind of lightweight wrapper around ERT to use ERT as a backend while providing nicer output and test grouping.

Anyway, if anyone would like to dig into Buttercup's recent changes and figure out why it's broken, that would certainly be appreciated. Otherwise, it seems that I will have to find time to do some serious yak shaving (or skinning, even).

@ivanperez-keera

This comment was marked as off-topic.

@yantar92

This comment was marked as off-topic.

@alphapapa

This comment was marked as off-topic.

@alphapapa

This comment was marked as off-topic.

@ivanperez-keera

This comment was marked as off-topic.

@ivanperez-keera

This comment was marked as off-topic.

@yantar92

This comment was marked as off-topic.

@ivanperez-keera

This comment was marked as off-topic.

@yantar92

This comment was marked as off-topic.

@hpfr

This comment was marked as off-topic.

@ivanperez-keera

This comment was marked as off-topic.

@alphapapa

This comment was marked as off-topic.

@ivanperez-keera
Copy link
Contributor

ivanperez-keera commented Jun 17, 2024

I've now added this change to my installation of org-ql (merging it individually as I have org-ql installed with elpa).

I have a file with more than 15 years worth of history. It won't be representative of all possible cases, but it's also not a trivial test.

It seems to be working well for me so far. I'll report back if I notice any issues.

@alphapapa
Copy link
Owner

I've now added this change to my installation of org-ql (merging it individually as I have org-ql installed with elpa).

I have a file with more than 15 years worth of history. It won't be representative of all possible cases, but it's also not a trivial test.

It seems to be working well for me so far. I'll report back if I notice any issues.

Thank you, I would appreciate that. My time for these projects has been limited lately, and ones that require careful review and testing like this are the hardest to find time for.

@ParetoOptimalDev
Copy link

I've now added this change to my installation of org-ql (merging it individually as I have org-ql installed with elpa).

I have a file with more than 15 years worth of history. It won't be representative of all possible cases, but it's also not a trivial test.

It seems to be working well for me so far. I'll report back if I notice any issues.

@ivanperez-keera If you can push your changes up, I can also test with 10+ years of history.

@ivanperez-keera
Copy link
Contributor

ivanperez-keera commented Jun 21, 2024

I'm not sure how to do that (I understand git, it's just that I'm using the version provided by ELPA so I don't know how that compares to your version).

It's only two lines that need to be adjusted. See: https://github.com/alphapapa/org-ql/pull/237/files#diff-34013f28e9b645ff9d9a6dab1c4f0ca707ae51e0361da84126e8440810452773R162-R163

It's quite easy to just add the change manually.

@ParetoOptimalDev
Copy link

It's only two lines that need to be adjusted. See: https://github.com/alphapapa/org-ql/pull/237/files#diff-34013f28e9b645ff9d9a6dab1c4f0ca707ae51e0361da84126e8440810452773R162-R163

This does match (todo) which wasn't before, but it doesn't seem to work with (scheduled :to today) or (ts-active :on today).

For example with that change this data:

*** TODO 15 minutes, standup
   SCHEDULED: <2024-06-24 Mon 09:00-09:15 ++7d>

Doesn't match (or (scheduled :to today) (ts-active :on today)).

@alphapapa
Copy link
Owner

@ParetoOptimalDev It works for me. It's likely that you did not load the changed library file correctly. i.e. defvar forms do not change the values of already-defined variables unless you individually set them with C-M-x, and doing that to one does not do that to others that depend on its value.

@alphapapa alphapapa force-pushed the patch/fix-regexp-part-ts-time branch 2 times, most recently from 0ac6b5e to f06cf3f Compare June 26, 2024 14:57
@alphapapa alphapapa force-pushed the patch/fix-regexp-part-ts-time branch from f06cf3f to e3b956e Compare June 26, 2024 15:25
@alphapapa alphapapa added this to the Future milestone Jun 26, 2024
@alphapapa
Copy link
Owner

v0.8.7 will allow such ranges to be matched simply, but matching depending on the specific inner time ranges will not work yet. That will require some refactoring, and so it will be done in a future version (maybe v0.9, maybe not).

@alphapapa alphapapa merged commit 4af08e3 into alphapapa:master Jun 26, 2024
exot added a commit to exot/.emacs.d that referenced this pull request Jun 30, 2024
alphapapa/org-ql#237 has been merged in 0.8.7,
so there should be no need anymore for my workaround.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for timestamps with internal time ranges (events)
9 participants