Skip to content

Date Searching #487

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

Merged
merged 26 commits into from
Mar 24, 2023
Merged

Conversation

chuck-flowers
Copy link
Contributor

An initial implementation of searching for headlines using date properties. This
PR requried a rewrite of the Search type to use a more traditional parsing
strategy internally. This allows for parsing more complicated queries more
easily, and also potential performance improvements since the query string is
parsed one time instead of every time the query is tested against a headline.

@chuck-flowers
Copy link
Contributor Author

I still need to support search values such as <today> and <+2d>, but given this change involves a pretty big rewrite I figured sharing my draft was a good idea.

I see that there are some checks failing so I'm going to look into that, in addition to supporting the features which I mentioned are still missing.

@chuck-flowers
Copy link
Contributor Author

I've fixed all the issues my code was having with the unit tests. I'll be moving on to supporting the "relative" date formats next.

@chuck-flowers chuck-flowers marked this pull request as ready for review January 25, 2023 13:01
@chuck-flowers
Copy link
Contributor Author

I think that this PR is finally ready for review. There are more changes here than I typically feel comfortable making as a single PR so I wanted to give a high-level summary.

I've updated the query parsing logic to use actual parsing logic rather than relying on Lua's pattern matching. This should make the parsing logic easier to follow and will allow us to more easily make changes to how it's parsed to be more compliant with Emacs moving forward. The core of this parsing logic could likely be moved to a utility module to be used in other parts of the application as well.

I updated the actual Search:check logic to used the parsed AST. I suspect there might be some minor performance benefits on larger queries across a large number of sections since the query is totally parsed one time up front. This also adds support for matching against dates in several formats that Emacs provides. These include:

  • Absolute dates (e.g. "<2023-01-25 Wed>")
  • Relative dates (e.g. "<-2w>")
  • "Keyword" dates (e.g. "")

I also added support for searching the DEADLINE, SCHEDULED, and CLOSED properties. Initially, I injected these into the property drawer during Section parsing, but that caused me to have to change several unit tests, so I decided to instead inject them into the Searchable model we were passing to Search:check instetad.

Take your time reviewing this PR. If you have any questions or find any bugs, please let me know and I will fix them as quickly as possible.

@jgollenz
Copy link
Contributor

jgollenz commented Mar 2, 2023

@chuck-flowers sorry for the long silence, I'll start reviewing this now.

@jgollenz
Copy link
Contributor

I only have superficial knowledge about parsers, but it looks good to me 👍 given the size of the MR, it's probably best @kristijanhusak also has a glance at it

Copy link
Member

@kristijanhusak kristijanhusak left a comment

Choose a reason for hiding this comment

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

Looks good so far.

@@ -54,10 +54,11 @@ describe('Search parser', function()
assert.Is.False(result:check({ tags = 'OTHER' }))
Copy link
Member

Choose a reason for hiding this comment

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

Why are changes to this test file needed? I don't see any changes in the Search class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry it's been a while since I did this development so I don't remember off hand. I'm trying to check compare my version of search_spec.lua to the current master version to see if anything breaks due to the feature I implemented. I'm having trouble getting tests to run though. I'm getting a lot of errors and it's apparently due to missing an orgmode TS grammar. I don't remember seeing this error before and I'm wondering if its related to the addition of the new folding logic.

I'd appreciate any guidance you can provide so I can give you a better answer for why these changes were necessary.

Copy link
Member

Choose a reason for hiding this comment

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

No worries. I don't think you introduced any changes that would break anything, you just added new behavior. I would suggest to start with reverting changes to the test files and go from there.
Regarding running tests locally, this should get you covered https://github.com/nvim-orgmode/orgmode#tests . This should work fine for Linux/MacOS. I'm not sure about Windows to be honest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've followed that procedure in the past without issue. Now I'm seeing the following errors in several test modules that I don't think my code interacts with.

For example, I'm seeing the following output as part of make test:

Testing:        /home/cflowers/.local/share/nvim/lazy/orgmode/tests/plenary/notifications/notifications_spec.lua
Fail    ||      Notifications should find headlines for notification
            /usr/share/nvim/runtime/lua/vim/treesitter/language.lua:32: no parser for 'org' language, see :help treesitter-parsers
            
            stack traceback:
                /usr/share/nvim/runtime/lua/vim/treesitter/language.lua:32: in function 'require_language'
                /usr/share/nvim/runtime/lua/vim/treesitter/languagetree.lua:35: in function 'new'
                ./lua/orgmode/parser/file.lua:173: in function 'from_content'
                ...gmode/tests/plenary/notifications/notifications_spec.lua:32: in function <...gmode/tests/plenary/notifications/notifications_spec.lua:18>
            
Fail    ||      Notifications should find repeatable and warning deadlines for notification
            /usr/share/nvim/runtime/lua/vim/treesitter/language.lua:32: no parser for 'org' language, see :help treesitter-parsers
            
            stack traceback:
                /usr/share/nvim/runtime/lua/vim/treesitter/language.lua:32: in function 'require_language'
                /usr/share/nvim/runtime/lua/vim/treesitter/languagetree.lua:35: in function 'new'
                ./lua/orgmode/parser/file.lua:173: in function 'from_content'
                ...gmode/tests/plenary/notifications/notifications_spec.lua:98: in function <...gmode/tests/plenary/notifications/notifications_spec.lua:76>
            
Fail    ||      Notifications should allow disabling specific reminder times
            ...gmode/tests/plenary/notifications/notifications_spec.lua:282: attempt to index local 'orgfile' (a nil value)
            
            stack traceback:
                ...gmode/tests/plenary/notifications/notifications_spec.lua:282: in function <...gmode/tests/plenary/notifications/notifications_spec.lua:277>
            
Fail    ||      Notifications should allow disabling specific reminder types
            ...gmode/tests/plenary/notifications/notifications_spec.lua:438: attempt to index local 'orgfile' (a nil value)
            
            stack traceback:
                ...gmode/tests/plenary/notifications/notifications_spec.lua:438: in function <...gmode/tests/plenary/notifications/notifications_spec.lua:433>
            

Success:        0
Failed :        4
Errors :        0

The first test mentions that there's no treesitter parse for the org language. Has something changed in the nvim-treesitter repository to cause this issue? Can you test with a fresh clone to see if it's reproducible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm no longer having this issue. It seems it may have been a bug published in Arch's tree-sitter package. I'll work on addressing this feedback now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've appended to this PR with a reversion of the test file back to master and then subsequent commits detailing why the minimal changes were necessary. All but one test is currently passing from the suite, but that test is also failing on master.

@@ -25,7 +25,28 @@ local time_format = '%H:%M'
---@field related_date_range Date
---@field dayname string
---@field adjustments string[]
local Date = {}
local Date = {
Copy link
Member

Choose a reason for hiding this comment

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

This is a very good change, I completely forgot about metamethods.

@chuck-flowers
Copy link
Contributor Author

@kristijanhusak I updated the test suite that you mentioned in your feedback

Copy link
Member

@kristijanhusak kristijanhusak left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you!

@kristijanhusak kristijanhusak merged commit 31fafad into nvim-orgmode:master Mar 24, 2023
@chuck-flowers chuck-flowers deleted the date-searching branch April 7, 2023 12:12
SlayerOfTheBad pushed a commit to SlayerOfTheBad/orgmode that referenced this pull request Aug 16, 2024
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.

3 participants