feat(graphql_formatter): implement BracketSpacing option

[denbezrukov]

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

[denbezrukov]

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

[Sec-ant]

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.

[ematipico]

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

[arendjr]

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.

[Sec-ant]

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.

[arendjr]

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.

[ematipico]

Weren't Partial meant to handle merging from extends?

[arendjr]

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.

[arendjr]

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

[arendjr]

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.

[codspeed-hq]

CodSpeed Performance Report

Merging #3310 will not alter performance

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

Summary

✅ 90 untouched benchmarks