alphapapa / org-ql

A searching tool for Org-mode, including custom query languages, commands, saved searches and agenda-like views, etc.
GNU General Public License v3.0
1.4k stars 110 forks source link

Handle the case when org-use-property-inheritance is a list of strings #346

Closed bram85 closed 9 months ago

bram85 commented 1 year ago

When org-use-property-inheritance is a list, then the query

(property "FOO" "BAR")

would bail out with:

org-ql--byte-compile-warning: Invalid Org QL query: "Invalid Org QL query: \"‘\\"BLAH\\"’ is a malformed function\", :warning", :error

(property "FOO") would still work though.

So, if org-use-property-inheritance is a list, cast it to the (default) boolean value for this variable: nil.

alphapapa commented 1 year ago

Hi Bram,

Thanks, but I'm not sure this is the correct solution to the problem. IIUC this means that, when the user configures the variable to be a list of properties to inherit, rather than those properties being inherited in the search, property inheritance will be disabled. What do you think?

Also, I don't fully understand your problem report, because I don't see BLAH anywhere in the query expression, only in the error.

bram85 commented 1 year ago

Thanks, but I'm not sure this is the correct solution to the problem. IIUC this means that, when the user configures the variable to be a list of properties to inherit, rather than those properties being inherited in the search, property inheritance will be disabled. What do you think?

I figured the :inherit keyword is expected to be a boolean value. And if org-use-property-inheritance is a list it cannot be handled so fall back to the variable's default value nil. I was guided by the docstring above that the boolean value of the variable is used if not given:

use the Boolean value of 'org-use-property-inheritance', which see (i.e. it is only interpreted as nil or non-nil)."

Also, I don't fully understand your problem report, because I don't see BLAH anywhere in the query expression, only in the error.

Indeed, I didn't mention it but this happens when org-use-property-inheritance is set to '("BLAH"). It doesn't have to be in the query to make it appear in the error message.

alphapapa commented 1 year ago

It does seem like there's a bug here, in that if org-use-property-inheritance is a list, an error is signaled.

But if the user sets the value to a list of properties that should be inherited, it would seem wrong to disable inheritance altogether, because that wouldn't produce the result the user expects.

alphapapa commented 1 year ago

The cause of this problem is that the org-ql-defpred macro doesn't account for the case in which the value of org-use-property-inheritance is a list; the expanded form includes the list value without quoting it, which the byte-compiler interprets as a function call, which isn't valid.

The correct solution would be to handle the case of the variable's value being a list by quoting the value in the macro expansion. Then the query should produce the expected result according to the value of the variable.

However, we should note that the variable's value may differ between buffers, and the query is not compiled for each buffer separately, so the best solution might be to have the property predicate's body form use the value of org-use-property-inheritance more "smartly", i.e. using its value directly when appropriate.

Generally it would be helpful if problems like this were first reported as bugs, so the problem could be fully understood before proposing a solution. :)

bram85 commented 1 year ago

I think it has been solved properly now by passing the selective symbol to org-entry-get in case org-use-property-inheritance is a list.

alphapapa commented 9 months ago

@bram85 Thanks for your patience. BTW, please avoid making PRs from your fork's master branch, as it makes it more difficult to rebase, etc.