Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat(properties): add option org_use_property_inheritance #880

Merged

Conversation

troiganto
Copy link
Contributor

@troiganto troiganto commented Jan 30, 2025

Summary

This PR adds the option org_use_property_inheritance and allows using property inheritance in searches and via the external API.

Related Issues

Extracted from #878

Changes

The code to search for a property recursively was already there, it's just that the default behavior couldn't be configured.

I've changed the behavior of get_property()'s parameter search_parents from two-state (truthy=yes, falsy=no) to tri-state (true=yes, false=no, nil=use config).

For the config itself, I've added the option itself and also a method OrgConfig:use_property_inheritance(), which evaluates it for a given property name.

I've adjusted the special cases where we want to always ignore (or always use) inheritance by grepping the code base for get_property and evaluating them one by one. I'm reasonably sure I got all uses, but it doesn't hurt to double check.

Checklist

I confirm that I have:

  • Followed the Conventional Commits specification (e.g., feat: add new feature, fix: correct bug, docs: update documentation).
  • My PR title also follows the conventional commits specification.
  • Updated relevant documentation, if necessary.
  • Thoroughly tested my changes.
  • Added tests (if applicable) and verified existing tests pass with make test.
  • Checked for breaking changes and documented them, if any.

@kristijanhusak
Copy link
Member

The code to search for a property recursively was already there, it's just that the default behavior couldn't be configured.

Yes, but I forgot to handle situations where we need all properties with get_properties(). There are 2 places:

  1. API - We expose all properties for a headline
  2. Search - This is used for matching in the tags agenda view

Since we already have inheritance with tags, there are two methods on headline class, get_tags and get_own_tags. Latter gets tags only from the headline itself.

Let's do the same thing, and update accordingly:

  1. rename current get_properties to get_own_properties in the OrgHeadline class
  2. Add new get_properties method that will fetch own priorities and all parents, and merge accordingly.
  3. On the 2 places I mentioned above use get_properties
  4. On every other place, rename it to get_own_properties

That should cover all the cases.

@troiganto troiganto force-pushed the feat/property_inheritance branch from 90e1c80 to c05873b Compare January 31, 2025 23:44
@troiganto
Copy link
Contributor Author

This seems reasonable! I've made the changes you suggested and added tests for both get_own_properties() and get_properties().

I've also added two TODO comments about some behavior that puzzled me somewhat. Let me know what you think about the annotated line and I can either remove them or fix it in a drive-by commit.

@troiganto troiganto changed the title Add option org_use_property_inheritance feat(properties): add option org_use_property_inheritance Jan 31, 2025
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. Let's just address that one TODO that you put, and I think that answers the question for your second TODO on the get_properties method.

@@ -543,6 +577,7 @@ memoize('get_tags')
function Headline:get_tags()
local tags, own_tags_node = self:get_own_tags()
if not config.org_use_tag_inheritance then
-- TODO: Why exclude the headline's own tags here?
Copy link
Member

Choose a reason for hiding this comment

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

Good catch! I think this is a bug. You can remove it.

@troiganto troiganto force-pushed the feat/property_inheritance branch from c05873b to e90e710 Compare February 1, 2025 21:03
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, thanks!

@kristijanhusak kristijanhusak merged commit 48f32e6 into nvim-orgmode:master Feb 2, 2025
6 checks passed
@troiganto troiganto deleted the feat/property_inheritance branch February 2, 2025 22:12
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants