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

(feat): implement parenthesis and boolean binary boolean operators. #251

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

natask
Copy link
Contributor

@natask natask commented Aug 26, 2021

Modify peg parser to support parenthesis and both and and or operators.

for example,
"love or (tags:happy and now) or later"
maps to

(or (or (regexp "love") (and (tags "happy") (regexp "now"))) (regexp "later"))

the default combining operator is set using `org-ql-default-predicate-boolean'.

The only issue is that both and and or are now operators and can't be used as query terms. In practice this is not a limitation. I never have searched for and or or.
It is possible to implement escaped versions of 'and' and 'or'. I propose using backlash to escape. \term to equate to term.

@natask
Copy link
Contributor Author

natask commented Aug 26, 2021

as a side point, This update allows for bijective mapping between sexp and string. Therefore org-ql--query-sexp-to-string' shouldn't need to return nil for any input. for example, currently org-ql--query-sexp-to-string' returns nil on

(org-ql--query-sexp-to-string '(or (or (regexp "love") (and (tags "happy") (regexp "now"))) (regexp "later")))

but with the above peg, it would be legal for it to return "love or (tags:happy and now) or later".

@alphapapa
Copy link
Owner

Hi again,

Well, I wrote that other comment before seeing this. :) Thanks, this is very cool. I'd been planning to do this myself eventually. It looks like you really dug into peg and figured out how to do it with only a few changes.

The only qualm I have is regarding the way multiple ORs seem to be combined, e.g. the way it works now is:

(or (or (regexp "love")
        (and (tags "happy")
             (regexp "now")))
    (regexp "later"))

But that's equivalent to:

(or (regexp "love")
    (and (tags "happy")
         (regexp "now"))
    (regexp "later"))

So it would be preferable to use the simpler expression, especially in cases where the sexp may be displayed to the user. I don't know how easy it would be to do that, and I guess it isn't absolutely necessary, but it would be nice to solve that.

What do you think? Thanks.

@natask
Copy link
Contributor Author

natask commented Aug 26, 2021

Now you mention it, I also prefer the simpler expression because it is easier to understand. As you said, I don't think it a necessary feature. I care mainly whether it works or not. On the point on aesthetics, from my experience with the expression displayed in org ql view, any sufficiently long expression looks unaesthetic because the sexp structure can not be communicated in a single line. I think better to display the string version for human consumption. every sexp now has a valid string version.

On implementing this, I think it would be easier to reduce the expression after it is parsed. It would require keeping state while parsing and I don't know how to do that with peg.

The following algorithm should work. After parsing, For each element recursively, if element is a list, check if parent list's operator matches element's operator, if it matches merge element into parent, removing the operator and the first level of brackets (like ,@) , else keep as it is.

@natask
Copy link
Contributor Author

natask commented Aug 26, 2021

as a tangential note, is there a way to debug closures bound to function variables with fset? I find myself needing to debug org-ql--normalize-query at times.

@alphapapa
Copy link
Owner

as a tangential note, is there a way to debug closures bound to function variables with fset? I find myself needing to debug org-ql--normalize-query at times.

I don't know. Does Edebug not work for it?

@natask
Copy link
Contributor Author

natask commented Aug 26, 2021

I don't know. Does Edebug not work for it?

Not as far as I know. I guess it is because it doesn't have a source file. I was hoping you would know.

@alphapapa
Copy link
Owner

If you edebug-defun on org-ql--def-query-string-to-sexp-fn and then call it appropriately, I think the closure will be "Edebugged" as well. That's how that sort of thing usually works IME.

@natask
Copy link
Contributor Author

natask commented Aug 26, 2021

thanks, it worked.

@alphapapa alphapapa added this to the 0.7 milestone Sep 22, 2021
@alphapapa alphapapa force-pushed the master branch 2 times, most recently from 4f5fbc4 to d0acc8c Compare May 30, 2022 16:27
@alphapapa
Copy link
Owner

FYI, it appears that manually optimizing away nested ors should be taken care of by the byte-compiler. For example, these two functions disassemble to the same bytecode:

(disassemble (byte-compile (lambda () (or t t))))
(disassemble (byte-compile (lambda () (or (or t t)))))

@alphapapa
Copy link
Owner

This will be a major enhancement and will need careful testing, so targeting for v0.8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants