This repository was archived by the owner on Sep 30, 2024. It is now read-only.
query parsing: Simplify parser and extract analysis function#50554
Merged
Conversation
Contributor
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff cfe99fb...f4b0a70.
|
camdencheek
approved these changes
Apr 14, 2023
Member
There was a problem hiding this comment.
Why was quoted removed from the Pattern interface?
Contributor
Author
There was a problem hiding this comment.
quoted and negated have always been set to false so they didn't seem to offer any value.
Member
|
sorry for the slow review! LGTM |
This commit aims to extract the "relevant tokens" logic to make it usable in other contexts. To be able to produce a valid token sequence from the parse tree with more confidence I'm changing the structure of the parse tree to - reuse existing pattern and filter tokens as leaves - make operators have a `left` and a `right` side instead of, theoretically, have an unlimited number of operands. In practice the `nodes` array has always at most two entries, but the current structure doesn't make that clear. Rather than implicitly relying on this fact when processing operator nodes I'm encoding this explicitly into the tree.
643739d to
a99f681
Compare
Bundle size report 📦
Look at the Statoscope report for a full comparison between the commits f4b0a70 and cfe99fb or learn more. Open explanation
|
fkling
added a commit
that referenced
this pull request
Apr 17, 2023
At the moment symbol suggestions will always insert 'type:symbol', which can produce incorrect queries if the current query already contains a 'type:' filter. This PR builds on top of #50554 and only inserts a 'type:' filter if the relevant branch of the query doesn't already contain one.
fkling
added a commit
that referenced
this pull request
Apr 17, 2023
At the moment symbol suggestions will always insert 'type:symbol', which can produce incorrect queries if the current query already contains a 'type:' filter. This PR builds on top of #50554 and only inserts a 'type:' filter if the relevant branch of the query doesn't already contain one. https://user-images.githubusercontent.com/179026/231516689-6c1b0a73-d25a-41cc-b032-e7a3d081ef5a.mp4 ## Test plan - Enter a query into the search input that triggers symbols suggestions and select a symbol suggestions -> the suggestion is inserted with `type:symbol` - Like above but also include e.g. `type:diff` in the query -> only the symbol name (not `type:symbol`) is inserted into the input ## App preview: - [Web](https://sg-web-fkling-search-input-symbol.onrender.com/search) Check out the [client app preview documentation](https://docs.sourcegraph.com/dev/how-to/client_pr_previews) to learn more.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
NOTE: Ignore white space changes for review
This commit aims to extract the "relevant tokens" logic to make it usable in other contexts. To be able to produce a valid token sequence from the parse tree with more confidence I'm changing the structure of the parse tree:
leftand arightoperand instead of, theoretically, have an unlimited number of operands. In practice thenodesarray has always at most two entries, but the current structure doesn't make that clear. Rather than implicitly relying on this fact when processing operator nodes I'm encoding this explicitly into the tree.Test plan
App preview:
Check out the client app preview documentation to learn more.