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_grit_formatter): initial infrastructure #2837

Merged
merged 5 commits into from
May 14, 2024

Conversation

abidjappie
Copy link
Contributor

@abidjappie abidjappie commented May 13, 2024

Summary

Set up the basic infrastructure required for further work on #2476

Implementation

  • Run just new-crate biome_grit_formatter
  • Update Cargo.toml with relevant settings
  • Update xtask/codegen/src/formatter
  • Run just gen-formatter

Test Plan

Cross comparison with other Biome formatters.

Copy link
Contributor Author

@abidjappie abidjappie left a comment

Choose a reason for hiding this comment

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

The code generator created src/js, do we need to keep this or can it be removed?

@abidjappie abidjappie mentioned this pull request May 13, 2024
28 tasks
@ematipico
Copy link
Member

The code generator created src/js, do we need to keep this or can it be removed?

This isn't correct. You have to update the codegen here:

// TODO: I will handle formatting in a follow-up PR.
LanguageKind::Grit => NodeConcept::Auxiliary,

@ematipico
Copy link
Member

It's possible you'll have to update this too:

enum NodeDialect {
Js,
Ts,
Jsx,
Json,
Css,
}

@github-actions github-actions bot added the A-Tooling Area: internal tools label May 13, 2024
@abidjappie
Copy link
Contributor Author

@ematipico Thank you for the feedback, as always!

I think I understand it a bit better now, although I am still a bit unclear on how the GritQL nodes are best categorized. I'll re-iterate again soon.

@abidjappie
Copy link
Contributor Author

abidjappie commented May 13, 2024

I'm not sure if covering all of these should be in the scope of this PR. (From the original issue)

Configuration

We need to add GritQL as a language in the configuration format, and let it support the common options around indentation, line length and type of newlines.

  • Add test suite
  • Introduce a can_format_grit_yet flag into biome_service
  • Comment placement
  • Handle biome ignore: format suppression comments in appropriate places

Additionally, I noticed that #1207 also added some additions like:

  • Readme
  • Git related
  • More...

Copy link
Contributor

@arendjr arendjr left a comment

Choose a reason for hiding this comment

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

Great start!

_ if name.contains("Operation") || name.contains("Pattern") => NodeConcept::Pattern,
_ if name.contains("Predicate") => NodeConcept::Predicate,
_ if name.ends_with("Definition") => NodeConcept::Declaration,
_ if matches!(name, "CodeSnippet") || name.ends_with("Literal") => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would've used name == "CodeSnippet" here ;)

@arendjr
Copy link
Contributor

arendjr commented May 14, 2024

I'm not sure if covering all of these should be in the scope of this PR

No, I think I'll just merge as is (if the remaining tests are green), and you can add options like this in follow-up PRs. That will allow us to iterate a bit faster, and you'll have to worry less about merge issues.

@arendjr
Copy link
Contributor

arendjr commented May 14, 2024

There just seems to be an issue with an unused dependency still.

Copy link

codspeed-hq bot commented May 14, 2024

CodSpeed Performance Report

Merging #2837 will not alter performance

Comparing abidjappie:feat/gritql-basic-infrastructure (94b4e8f) with main (f847aff)

Summary

✅ 97 untouched benchmarks

@ematipico ematipico merged commit 14d196f into biomejs:main May 14, 2024
10 of 12 checks passed
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