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(parser): implement GritQL parsing #1922

Closed
wants to merge 12 commits into from

Conversation

arendjr
Copy link
Contributor

@arendjr arendjr commented Feb 26, 2024

Summary

Based on the open-source GritQL grammar, this PR implements a parser for GritQL in Biome.

I'll admit I had underestimated how much manual parsing logic needed to be implemented, but I've gotten the hang of it and progress is moving fast now.

It's nice that the codegen also seems to generate most of the formatter already, but I have to take a closer look still to see it really works.

TODO:

  • Implement maps and lists
  • Implement assignments
  • Implement definitions
  • Refine error resilience? Maybe @ematipico or @Conaclos can have a look if I'm doing it the right way
  • More test cases
  • Finish up formatter

Test Plan

Still adding test cases.

@github-actions github-actions bot added A-Parser Area: parser A-Tooling Area: internal tools L-JSON Language: JSON and super languages labels Feb 26, 2024
Copy link

netlify bot commented Feb 26, 2024

Deploy Preview for biomejs canceled.

Name Link
🔨 Latest commit c948070
🔍 Latest deploy log https://app.netlify.com/sites/biomejs/deploys/65e0ed32cbc4a10008ed4d47

Copy link

codspeed-hq bot commented Feb 26, 2024

CodSpeed Performance Report

Merging #1922 will degrade performances by 8.14%

Comparing arendjr:gritql_parser (c948070) with main (78dc430)

Summary

❌ 1 regressions
✅ 92 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main arendjr:gritql_parser Change
eucjp.json[uncached] 5.1 ms 5.5 ms -8.14%

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

I need to ask you to break down this PR in smaller PRs. It's impossible to review it. Sorry about that.

Some advice on how to break it down:

  1. start with a PR of the grammar and its code generation;
  2. then a PR that contains the parsing only;
  3. the formatter is not needed, the code-generation of the formatter is done a different step, which can be ignored. If you wish to add it, it can be done in a separate PR

@arendjr
Copy link
Contributor Author

arendjr commented Feb 27, 2024

No worries! I think I'll first get the parser complete, since at least initially I sometimes needed to tweak the grammar based on what I discovered implemented the parser. It's probably mostly stable by now, but if you don't mind I'll just use this one to track my progress, and then split things to get it reviewed and merged.

@arendjr
Copy link
Contributor Author

arendjr commented Feb 29, 2024

I created test cases out of most of the snippets in the Grit docs and they all parse. Still have some FIXMEs in there and I think I might have some precedence issues still, but next step will be splitting this PR :)

@arendjr arendjr mentioned this pull request Mar 1, 2024
@arendjr
Copy link
Contributor Author

arendjr commented Mar 7, 2024

Obsoleted by #1998 .

@arendjr arendjr closed this Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Parser Area: parser A-Tooling Area: internal tools L-JSON Language: JSON and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants