-
-
Notifications
You must be signed in to change notification settings - Fork 482
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(biome_graphql_parser): parse schema definition #2557
feat(biome_graphql_parser): parse schema definition #2557
Conversation
CodSpeed Performance ReportMerging #2557 will not alter performanceComparing Summary
|
} | ||
|
||
fn is_at_list_end(&self, p: &mut Self::Parser<'_>) -> bool { | ||
is_at_root_operation_type_definition_end(p) |
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.
Could you please help me to understand why we need here is_at_root_operation_type_definition_end
?
What cases do we cover? Is it possible to check only p.at(T!['}'])
?
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.
Consider this case:
schema {
query: MyQueryRootType
mutation: MyMutationRootType
schema {
query: Query
}
If we only check for p.at(T!['}'])
the second schema definition is also included in RootOperationType, so I included another check to see if we are at the start of other definition or not.
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.
Do you think this is a good idea?
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 believe it's a great idea.
I'm wondering if it's possible to have a 'complex' check when we get an Absent
result from the parse_element
function.
Specifically, it would be beneficial to maintain p.at(T!['}'])
the happy path, avoiding additional checks which are required for the failure path.
I don't have any work solution for now, we might think about it in the next MR.
|
||
#[inline] | ||
fn parse_root_operation_type_definition(p: &mut GraphqlParser) -> ParsedSyntax { | ||
if !(p.at_ts(OPERATION_TYPE) || p.at(T![:]) || p.lookahead_at(T![:])) { |
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 believe we can extract p.at_ts(OPERATION_TYPE) || p.at(T![:]) || p.lookahead_at(T![:])
as a is_at_root_operation_type_definition
function and reuse it in the RootOperationTypeDefinitionListRecovery
.
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.
🚀🚀🚀
3db791d
to
00dc3b2
Compare
00dc3b2
to
42c0b7f
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.
Nice job!
Summary
Parse GraphQL schema definition. This is a small case so I parse both the happy path and error path in this PR
Test Plan
All tests should pass