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

feat(biome_graphql_parser): parse object extension #2928

Conversation

vohoanglong0107
Copy link
Contributor

Summary

Parse GraphQL object type extensions (link). This is a small case, as almost all parsing logic was handled by a previous PR focusing on parsing object type definition.
The object extension syntax defined in the ungrammar was also needlessly complicated, so this PR also simplifies the syntax.

Test Plan

All tests should pass

@github-actions github-actions bot added the A-Tooling Area: internal tools label May 21, 2024
Copy link

codspeed-hq bot commented May 21, 2024

CodSpeed Performance Report

Merging #2928 will not alter performance

Comparing vohoanglong0107:feat-graphql-parse-object-type-extension (61744b6) with main (8f3c811)

Summary

✅ 92 untouched benchmarks

@vohoanglong0107 vohoanglong0107 marked this pull request as ready for review May 21, 2024 06:04
@ematipico
Copy link
Member

PR has conflicts

pub(crate) fn parse_object_type_extension(p: &mut GraphqlParser) -> ParsedSyntax {
let m = p.start();

p.bump(T![extend]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bump can panic if the current token doesn't match.
It's better to use expect or have a guard like is_at_object_type_extension

Copy link
Contributor Author

@vohoanglong0107 vohoanglong0107 May 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have placed a check on the outer parse_extension function here, so unless this function was called outside of parse_extension it should be fine. I think panic would even be preferred in this case, cause it will fail the test and indicate that this function was called at a unintended location. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'm unsure how to handle this better because I understand the cons and pros of both approaches.
I see two ways:

  1. Add is_at_object_type_extension and use it only in parse_object_type_extension. This brings some duplication because we already check the correct parser state beforehand. However, I tend to use this approach.

  2. Write documentation stating that we must check extends and type on the call sites; otherwise, it can cause a panic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added documents to check for the tokens. Since this pattern is also used in some other places, I have also documented them as well.

@vohoanglong0107 vohoanglong0107 force-pushed the feat-graphql-parse-object-type-extension branch from a8d390d to 865a097 Compare May 21, 2024 08:05
@ematipico ematipico merged commit 35702c4 into biomejs:main May 21, 2024
13 checks passed
@vohoanglong0107 vohoanglong0107 deleted the feat-graphql-parse-object-type-extension branch May 21, 2024 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Tooling Area: internal tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants