-
-
Notifications
You must be signed in to change notification settings - Fork 465
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
Add GritQL grammar #1951
Add GritQL grammar #1951
Conversation
✅ Deploy Preview for biomejs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
CodSpeed Performance ReportMerging #1951 will not alter performanceComparing Summary
|
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 skimmed the grammar, and I found some inconsistencies and errors that should be addressed before merging.
- comments with example: they are important for the review and provide context
- bogus nodes: I noticed that there's some misuse of the bogus nodes. Usually they are always part of a
Any*
node, and they are never part of the definition of a specific node - naming: all nodes must start with
Grit*
unless they are known nodes - inconsistencies: some nodes are defined like
Node = 'literal'
, which isn't a pattern we use throughout our grammars - lists: they should always be NOT optional
xtask/codegen/gritql.ungram
Outdated
'language' | ||
name: GritLanguageName | ||
flavor: GritLanguageFlavor? | ||
GritBogus? |
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.
A bogus should not be in this position. Usually bogus nodes are part of a Any**
node.
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.
It helped me to implement patterns such as this in the parser:
let _ = parse_pattern(p).or_recover_with_token_set(
p,
&ParseRecoveryTokenSet::new(GRIT_BOGUS_PATTERN, PATTERN_RECOVERY_SET),
expected_pattern,
);
Without this, it appeared as though the bogus nodes wouldn't automatically "bubble up" to the next Any**
node. I suppose that might simply be because I had made some other mistake, what steps do I need to follow to make sure the Any*
nodes can consume these?
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.
There aren't any particular steps to follow, really - other than the fact that a bogus node must be part of a Any*
node. The recovering logic is up to us entirely, and we decide what to wrap in bogus node. We just make judgement calls based on the DX we want to offers to our tools (formatter, linter, etc.).
By looking at the snippet of the parser, I would expect a grammar like this:
GritNode =
some_node: GritSomeNode
pattern: AnyGritPattern
AnyGritPattern =
GritBogusPattern
GritSpecificPattern
This means when you're parsing the pattern
node, you have to account any possible errors, and make sure the parser is able to recover itself and emit a GRIT_BOGUS_PATTERN
.
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.
Thanks, maybe I was just trying to do the recovery in the wrong node then.
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.
If you have questions, @denbezrukov is an expert in the field :)
Think I've processed them all :) |
Is the unused dependency lint safe to ignore in this case? Or how would I ignore it until the parser PR is ready? |
Something didn't go as expected, I advise you to check why to check what is causing the warning. When I created the HTML grammar, I didn't get any warning. Unless there's something more that I missed |
Thanks! I misread the warning about the unused dependencies. I thought it complained because |
Summary
This implements the GritQL grammar based on my original (oversized) PR: #1922
This is just the grammar and the
grit_kinds_src.rs
along with the accompanying generated files and the minimal set of changes to make everything compile again. The actual parser will follow in the next PR.Test Plan
CI should remain green.