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

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

Closed
wants to merge 3 commits into from

Conversation

bram85
Copy link
Contributor

@bram85 bram85 commented Apr 12, 2023

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.

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
Copy link
Owner

alphapapa commented Apr 12, 2023

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
Copy link
Contributor Author

bram85 commented Apr 12, 2023

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
Copy link
Owner

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
Copy link
Owner

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. :)

@alphapapa alphapapa self-assigned this Apr 12, 2023
@alphapapa alphapapa added the bug label Apr 12, 2023
@alphapapa alphapapa added this to the 0.8 milestone Apr 12, 2023
This symbol is mentioned in the org-entry-get documentation.
@bram85
Copy link
Contributor Author

bram85 commented Apr 16, 2023

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.

Release: v0.7.2

# -----BEGIN PGP SIGNATURE-----
#
# iF0EABECAB0WIQRibGN7s6Ag030zGsLn4Z3JMOysmwUCZRO8JAAKCRDn4Z3JMOys
# mxpgAKDB9mCBhmsIfMAgQVRCcAOjBu6A7gCfQRSwZyBQYXIFwJ/y9JA2Ftpkafw=
# =w2+9
# -----END PGP SIGNATURE-----
# gpg: directory '/data/data/com.termux/files/home/.gnupg' created
# gpg: Signature made 2023-09-27 07:22:44 +0200 CEST
# gpg:                using DSA key 626C637BB3A020D37D331AC2E7E19DC930ECAC9B
# gpg: Can't check signature: No public key
@alphapapa alphapapa closed this in 9a5680c Dec 16, 2023
@alphapapa
Copy link
Owner

@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.

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.

2 participants