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(graphql_formatter): implement BracketSpacing option #3310

Merged
merged 7 commits into from
Jun 30, 2024

Conversation

denbezrukov
Copy link
Contributor

Summary

We have the BracketSpacing option for JavaScript files. We can reuse this option for GraphQL files. The main idea of the PR is to move BracketSpacing to the global formatter option so that we can use it for both languages.

Since I'm not very familiar with this part of the code, I'd like to ask for help in reviewing.

Test Plan

cargo test

@github-actions github-actions bot added A-CLI Area: CLI A-Project Area: project A-Formatter Area: formatter L-JavaScript Language: JavaScript and super languages L-CSS Language: CSS L-JSON Language: JSON and super languages labels Jun 29, 2024
@denbezrukov denbezrukov requested review from a team June 29, 2024 09:17
@@ -55,7 +55,12 @@ pub struct GraphqlFormatter {
argument("double|single"),
optional
))]
pub quote_style: QuoteStyle,
Copy link
Contributor Author

@denbezrukov denbezrukov Jun 29, 2024

Choose a reason for hiding this comment

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

Why do we have some options as Option<T> and others as <T>?

Copy link
Member

@Sec-ant Sec-ant Jun 29, 2024

Choose a reason for hiding this comment

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

We recently refactored some of the code (JSON and CSS especially) to use PartialXXXConfiguration when mapping the options to LanguageSettings. In a Partial-prefixed struct every option is an Option<T>. But this is yet to be done for the Js language, see #3297, and maybe GraphQL as it is newly added. Until the refactoring is completed, using Option<T> should be a more safe choice.

And honestly, I don't know if there're cases where we shouldn't use Partial-prefixed structs, and am not very clear why we introduce the partial macro if every option is already wrapped in an Option. I'm also not clear why we use the partial macros for primitive values.

Copy link
Member

Choose a reason for hiding this comment

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

@arendjr would you mind giving us some background here? I admit that I also lack the knowledge, so maybe we should have it somewhere

Copy link
Contributor

Choose a reason for hiding this comment

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

So the goal of the Partial types is to enable merging of configs. With fields that are not wrapped in Option you may not know if a field is set and just happened to be set to the default value or if it was not set at all. The distinction matters because when merging one should override values in the config it is being merged into, while the other shouldn’t.

Configurations that are already merged don’t need to use Partial types anymore, and their fields generally don’t need to be wrapped in Option as long as there’s a sensible default. Of course, sometimes it may still make semantic sense to use an Option wrapper for given field, and I think in such a case the derived Partial type won’t wrap the Option again.

For primitive types, they indeed don’t need to have a Partial version, but they still implement the Mergeable trait (I’m not entirely sure about the trait name anymore) so that the derive macro can treat all types consistently.

We recently refactored some of the code (JSON and CSS especialy) to use PartialXXXConfiguration when mapping the options to LanguageSettings

What was the goal of this? When creating the Partial types the idea was that XXXConfiguration could define all the default values and would be the result of the merge process, so PartialXXXConfiguration would only be used before merging. I suppose you could indeed go directly from Partial types to Settings but you’ll need a different place to define the defaults I guess. I’m not very partial (pun intended) to which approach we use, as long as we’re aligned we go the right direction.

Copy link
Member

Choose a reason for hiding this comment

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

When creating the Partial types the idea was that XXXConfiguration could define all the default values and would be the result of the merge process, so PartialXXXConfiguration would only be used before merging. I suppose you could indeed go directly from Partial types to Settings but you’ll need a different place to define the defaults I guess. I’m not very partial (pun intended) to which approach we use, as long as we’re aligned we go the right direction.

In our current infra structure we map Configuration (direct input) to Settings (our internal workspace struct), then to Options (parse/format/lint functions). The reason we need the Configuration to be partial is that the overrides are calculated in the Settings phase, so we need the None type to know whether an option can override the existing one. So in #3272 and #3273 we change all fields in the Settings to Option<T>, this will also require us to use the PartialXXXConfiguration for mapping options to Settings.

We only keep concrete/merged fields in the Options structs.

But I still think we should keep the defaults in the Configuration structs though, and wrap all the primitives with custom types so we can provide custom defaults. Just a half-baked idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I guess if we put the defaults in Settings and make sure all the Configuration fields are all Options already we can drop the whole Partial concept since it becomes kinda redundant at that point. Think that might actually simplify things.

Copy link
Member

Choose a reason for hiding this comment

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

Weren't Partial meant to handle merging from extends?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but we also still have a separate trait for that IIRC. The Partial types are just to enforce that all fields are wrapped in Option. But if we want to forward everything as an Option to the Settings anyway, I think it might be sufficient to document it like that and we may not need the separate macro anymore.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just checked, it’s the Merge derive macro that does the actual merging for the extends feature.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we really want enforcement maybe we could even let the Merge macro raise an error if a field isn’t an Option, with a message explaining why. But then at least we don’t need separate types for it anymore.

Copy link

codspeed-hq bot commented Jun 29, 2024

CodSpeed Performance Report

Merging #3310 will not alter performance

Comparing feat/bracket-spacing (b3d5353) with main (d785012)

Summary

✅ 90 untouched benchmarks

crates/biome_cli/src/commands/rage.rs Outdated Show resolved Hide resolved
crates/biome_cli/src/commands/rage.rs Show resolved Hide resolved
crates/biome_cli/src/commands/rage.rs Outdated Show resolved Hide resolved
denbezrukov and others added 4 commits June 30, 2024 07:33
Co-authored-by: Ze-Zheng Wu <zezhengwu@proton.me>
Co-authored-by: Ze-Zheng Wu <zezhengwu@proton.me>
Co-authored-by: Ze-Zheng Wu <zezhengwu@proton.me>
@denbezrukov denbezrukov merged commit f741a13 into main Jun 30, 2024
15 checks passed
@denbezrukov denbezrukov deleted the feat/bracket-spacing branch June 30, 2024 07:00
rishabh3112 pushed a commit to rishabh3112/biome that referenced this pull request Jul 6, 2024
Co-authored-by: Ze-Zheng Wu <zezhengwu@proton.me>
@Conaclos Conaclos added the A-Changelog Area: changelog label Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-CLI Area: CLI A-Formatter Area: formatter A-Project Area: project L-CSS Language: CSS L-JavaScript Language: JavaScript and super languages L-JSON Language: JSON and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants