-
Notifications
You must be signed in to change notification settings - Fork 117
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: Add basic url filter parser #5653
Conversation
d3e1d46
to
01f1ed7
Compare
2c9aff7
to
6c33b95
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks good, but left various comments/questions throughout. Additionally, for posterity, a README.md file in the url-state/filters
directory would be helpful to explain our filter language, nearly, and how to generate the parser.
web-common/src/features/dashboards/url-state/filters/convertors.ts
Outdated
Show resolved
Hide resolved
web-common/src/features/dashboards/url-state/filters/convertors.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when the string cannot be properly parsed? Can we add some test cases for the failure cases to show what happens? (Given user-provided strings will certainly fail occasionally)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a couple to demonstrate the error. We should get a UX pass eventually.
web-common/src/features/dashboards/url-state/filters/convertors.ts
Outdated
Show resolved
Hide resolved
web-common/src/features/dashboards/url-state/filters/convertors.ts
Outdated
Show resolved
Hide resolved
web-common/src/features/dashboards/url-state/filters/convertors.ts
Outdated
Show resolved
Hide resolved
web-common/src/features/dashboards/url-state/filters/convertors.ts
Outdated
Show resolved
Hide resolved
2ed3eda
to
0f69fc7
Compare
| (boolean_expr _ "AND"i _):+ non_and_expr {% andOrPostprocessor %} | ||
| (boolean_expr _ "OR"i _):+ non_or_expr {% andOrPostprocessor %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be clearer to have a discrete andPostprocessor
and an orPostprocessor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They have the exact same implementation. This is the suggested usage of post post processors, being able to reuse them. Feel like it would be redundant.
@builtin "whitespace.ne" | ||
@builtin "number.ne" | ||
@builtin "string.ne" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are these referenced? I don't see any explicit references, but I'm guessing maybe the primitive types at the bottom (e.g. sqstring
, int
, decimal
) are coming from here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the docs arent here for these right now. They point to the source code of these files: https://github.com/kach/nearley/tree/master/builtin
# these are used to disambiguate matches | ||
non_and_expr => boolean_expr {% id %} | ||
| (boolean_expr __ "OR"i __):+ non_and_expr {% andOrPostprocessor %} | ||
non_or_expr => boolean_expr {% id %} | ||
| (boolean_expr __ "AND"i __):+ non_or_expr {% andOrPostprocessor %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not totally following these. At first glance, it looks like adding these conditions would prevent multiple "ANDs" and multiple "ORs" in the same statement. But I see in the test cases that "X and Y and Z" is possible. So, mind just clarifying in the comment what these do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are used in expr
(boolean_expr _ "AND"i _):+ non_and_expr
for AND. It matches continuous AND ended by a non AND expr.
Added some comments around this.
For human readable url params we need a parser for filter param.
Possible expressions,
V1Expressioin