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

Consider adopting a parsing library for query lines to replace hard-to-maintain regular expressions #2079

Open
claremacrae opened this issue Jun 30, 2023 · 6 comments
Labels
scope: filters Additions and modifications to the search filters type: internal Only regards development or contributing

Comments

@claremacrae
Copy link
Collaborator

Description

Problem

We have a number of bug reports around problems interpreting complex query lines. For example:

Idea

I have been thinking for some time that as we add more features, our regular expressions are getting more complex.

And it would be worth investigating whether re-using an existing parsing solution would be more sustainable, long-term.

Options

I asked in OMG Discord - #plugin-dev on 2023-05-29, and the replies were:

Required Steps

  • Establish our requirements for parsers

  • Pick a library

    • Review the options listed above (and any more found since)
    • See how much they increase release distribution size by
    • Show what the code would look like in Tasks
  • Start migrating code, incrementally.

How to Test

Confirm that the existing Jest tests and smoke tests all work.

@claremacrae claremacrae added type: internal Only regards development or contributing scope: filters Additions and modifications to the search filters labels Jun 30, 2023
@esm7
Copy link
Contributor

esm7 commented Jun 30, 2023

We do have a parsing library, boon-js. The regular expressions are about preparing the expressions to be processable by that library. And while it is surely preferable to not have them, while searching for viable solutions at the time I checked a few directions, and my conclusion was that all the parsers that I found are considerably harder to use and would require considerably more code (and formal language definitions, which are not much better than cryptic regex) to maintain.
That being said, this research is still very worth doing. It's definitely possible that better solutions are out there, either ones that I reviewed, did not find, or maybe even progressed in the year that has passed since the original implementation :)

@claremacrae
Copy link
Collaborator Author

We do have a parsing library, boon-js.

😄

(and formal language definitions, which are not much better than cryptic regex)

Yes, I'm very conscious of that too.

while searching for viable solutions at the time I checked a few directions

That's great to hear, @esm7...

If you happened to have any notes about the ones you investigated, would you mind dropping them in here when you have time...

@esm7
Copy link
Contributor

esm7 commented Jun 30, 2023

If you happened to have any notes about the ones you investigated, would you mind dropping them in here when you have time...

Will try to dig up the relevant notes. I found the note where I found and decided upon boon-js, but I did not properly link it to the previous alternatives (one previous alternative I actually wrote a full implementation for), so it's much harder to find than it should have been 😞
(luckily my note-taking practices have vastly improved in the meantime)

@sbliven
Copy link
Contributor

sbliven commented Oct 13, 2023

Even if it's not used for parsing, defining the grammar for tasks queries would be good documentation.

I suspect that creating a clean grammar will require changing the behavior of some edge cases. What level of breaking changes are acceptable for the project?

@claremacrae
Copy link
Collaborator Author

What level of breaking changes are acceptable for the project?

For accidental behaviour that was not documented, then changes in behaviour where the user is informed, those would be OK. (For example, a new error message that says “this used to do X, now that doesn’t work anymore - change your query to Y instead”)

For documented behaviour, I would want to discuss it on a case-by-case basis, but the default answer would be “none”.

@claremacrae
Copy link
Collaborator Author

When specific changes are discovered, it will definitely be worth discussing them.
For example, if a filter started matching more tasks, that would be more acceptable than one starting to match fewer tasks. Missing results being a lot more serious for users than extra ones, as a general principle of search software…/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: filters Additions and modifications to the search filters type: internal Only regards development or contributing
Projects
None yet
Development

No branches or pull requests

3 participants