[go: up one dir, main page]
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.

Sign up for GitHub

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

Jump to bottom

Plain query doesn't handle multiple conditions correctly #314

Closed
akirak opened this issue Dec 4, 2022 · 4 comments
Closed

Plain query doesn't handle multiple conditions correctly #314

akirak opened this issue Dec 4, 2022 · 4 comments

Comments

@akirak
Copy link
Contributor
akirak commented Dec 4, 2022 •
edited
Loading

Hello, I'm using org-ql-completing-read, but it doesn't seem to handle multiple conditions now.

Single condition seems to work:

ELISP> (org-ql--query-string-to-sexp "todo: ")
(todo)

However, if multiple conditions are specified in a plain query, all queries other than the first one become a rifle query with the first letter eliminated:

ELISP> (org-ql--query-string-to-sexp "todo: test")
(and
 (todo)
 (rifle "est"))
ELISP> (org-ql--query-string-to-sexp "todo: ts:on=today")
(and
 (todo)
 (rifle "s:on=today"))
ELISP> (org-ql--query-string-to-sexp "sample text")
(and
 (rifle "sample")
 (rifle "ext"))

It once behaved differently from this. Is this the current expected behavior?
@akirak akirak mentioned this issue Dec 4, 2022
Closed
@alphapapa
Copy link
Owner

That's not what I get. I get this:

(org-ql--query-string-to-sexp "todo: test")
;; (and (todo) (rifle "test"))

(org-ql--query-string-to-sexp "todo: ts:on=today")
;; (and (todo) (ts :on "today"))

(org-ql--query-string-to-sexp "sample text")
;; (and (rifle "sample") (rifle "text"))

@akirak
Copy link
Contributor Author
akirak commented Dec 6, 2022 •
edited
Loading

There was a recent update in peg.el: https://github.com/emacsmirror/peg/commit/90c9004a8347209f94a37c8d9506425721098402

The test cases for the plain query parsing succeed with the previous version of peg.el but fail after the update. It was the same phenomenon as I described above:

Plain query parsing
Negated terms Expected (org-ql--query-string-to-sexp "todo: !todo:CHECK,SOMEDAY")' to be equal' to (and (todo) (not (todo "CHECK" "SOMEDAY")))', but instead it was (and (todo) (todo "CHECK" "SOMEDAY"))' which does not match because: (list-elt 2 (proper-lists-of-different-length 3 2 (todo "CHECK" "SOMEDAY") (not (todo "CHECK" "SOMEDAY")) first-mismatch-at 0)). (28.73ms)
Regexp predicates (11.42ms)
Timestamp-based predicates (28.56ms)
To-do predicates (8.93ms)
Compound queries Expected (org-ql--query-string-to-sexp "todo:SOMEDAY ts-a:from=2020-01-01,to=2021-01-01")' to be equal' to (and (todo "SOMEDAY") (ts-a :from "2020-01-01" :to "2021-01-01"))', but instead it was (and (todo "SOMEDAY") (regexp "s-a:from=2020-01-01"))' which does not match because: (list-elt 2 (proper-lists-of-different-length 2 5 (regexp "s-a:from=2020-01-01") (ts-a :from "2020-01-01" :to "2021-01-01") first-mismatch-at 0)). (3.78ms)

So the issue was due to the update in peg.

I have reproduced the issue using my own framework here. The test cases only fail after updating peg.el at akirak@5cc32f5:

@akirak akirak closed this as completed Dec 6, 2022
@akirak akirak mentioned this issue Dec 6, 2022
Closed
alphapapa added a commit that referenced this issue Dec 9, 2022
@alphapapa
Fix: (org-ql--def-query-string-to-sexp-fn) Updated for peg v1.0.1
9f00fa3 
Fixes #314, fixes #316.  Thanks to Akira Komamura (@akirak) and Joon
Ro (@joonro) for reporting.

Instead of `[blank]` in this PEX, it used to be `(syntax-class
whitespace)`, which worked fine with peg v1.0.  Then
<https://debbugs.gnu.org/cgi/bugreport.cgi?bug=59345> was filed and
resulted in the release of peg v1.0.1, after which that no longer
worked.  As best I can tell from reading the discussion, `[blank]` and
`(syntax-class whitespace)` should behave the same way, yet here they
do not: with peg v1.0.1, only `[blank]` works.  Why, I do not know; I
only tried it because I could find no explanation for the broken
behavior, and luckily, it works.  Maybe it's due to the use of
`(syntax-class whitespace)` later in the `pexs`; but since it finally
works again, let's take the victory as-is.
@alphapapa
Copy link
Owner

This should be fixed now. Please let me know if it works for you.

@akirak
Copy link
Contributor Author
akirak commented Dec 9, 2022

It works with the latest version of peg from ELPA. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@alphapapa @akirak