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

Adds support for :LOGGING: property #584

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

chuck-flowers
Copy link
Contributor

Addresses issue #466 by adding support for the :LOGGING: property to the note/timestamp insertion logic when updating the TODO state of tasks.

I factored out the parsing framework I created for adding date property support to the agenda search. I used this framework to parse the syntax that orgmode describes in its manual for the :LOGGING: property. I then, determined whether to insert a timestamp or note by first checking for the :LOGGING: property on the affected section and then using the property defined in the user's orgmode config.

@kristijanhusak kristijanhusak self-requested a review June 27, 2023 09:08
@chuck-flowers
Copy link
Contributor Author

I just realized that master has moved forward since I opened the PR. I can rebase this onto master unless that will cause a problem

@chuck-flowers
Copy link
Contributor Author

I've rebased on the latest master since git was reporting that there was a merge conflict

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.

Thanks for the PR! And sorry for the late review.
The code looks generally ok, but as I mentioned in one of the comments, I think we cannot do file/headline level todo keywords at this moment. It needs many more changes in order to work properly.
Adding support for @ and ! in the todo keywords is already a great addition, so I would like to scope this PR to that.

@@ -0,0 +1,69 @@
local M = {}
Copy link
Member

Choose a reason for hiding this comment

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

Let's move this file to lua/orgmode/parsers/utils.lua.

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 renamed the file as you've requested

local logging_prop = item.properties.items.logging
if logging_prop == 'nil' then
section_config = false
elseif logging_prop ~= nil then
Copy link
Member

Choose a reason for hiding this comment

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

Having headline or file scoped config for todo keywords requires many more changes across the codebase to be properly considered. It will work on state change, but agenda, highlights, etc. will not work as expected.

Can we have this PR only handle @ and ! in the todo keywords and leave the file/headline level todo keywords for a different PR?

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 reverted the 2 commits which targeted the mappings file. If I remember right, this shouldn't affected parsing the "global" config value, but I will reconfirm this with my personal usage tomorrow.

Updates the type annotation for the `DefaultConfig` type to specify the
allowed values of org_log_done for the Lua LSP. The values that are
specified are based on the currently documented values for `org_log_done`.
Adds a method to the `TodoConfig` type to make determining whether to
log a time, note, or not log at all for a give TodoConfig easier.
Remove the LOGGING property logic from the sections and the file-level.
@chuck-flowers
Copy link
Contributor Author

@kristijanhusak I think I have addressed the requested feedback. Please let me know if I have missed something

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.

@chuck-flowers This PR now just introduces a TodoConfig parser without actually using it? Is that intended?
I thought we will incorporate it to work with the todo keywords defined in the global configuration.
I'm ok doing that separately, and have this only as a refactor + preparation for the next PR. Just want to confirm that is the intention.

Could we have a unit test that tests todo-config file to ensure it parses everything properly?

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