Consider making fsharp_experimental_stroustrup_style apply independently of fsharp_multiline_block_brackets_on_same_column

[josh-degraw]

As mentioned in a comment in #2413:

Using fsharp_experimental_stroustrup_style without fsharp_multiline_block_brackets_on_same_column does not make any sense. ExperimentalStroustrupStyle is an extension of MultilineBlockBracketsOnSameColumn.

Currently, to use the experimental Stroustrup setting you need to enable two flags. Other than the potential issue of not knowing you need to enable two flags, the MultilineBlockBracketsOnSameColumn is technically inaccurate when using the Stroustrup style, as by definition the brackets are on different columns.

I propose that:

• If fsharp_experimental_stroustrup_style is enabled, fsharp_multiline_block_brackets_on_same_column should be treated as redundant and relevant logic should be applied automatically whether it's present or not.
• Logic that would format differently when using Stroustrup vs MultilineBlockBracketsOnSameColumn should only need to check if the Stroustrup setting is enabled,
• Logic that would apply identically for either Stroustrup or MultilineBlockBracketsOnSameColumn, but not otherwise, should check if either flag is present instead of only being applied if MultilineBlockBracketsOnSameColumn is true.

Alternatively, it might make sense to have a different flag to encapsulate the possible bracket styles (e.g. stroustrup, allman, classic, or whatever is most accurate). Admittedly a change like that could balloon out in logic handling, but for configuring the code it could make it much more straightforward to only need one flag to represent all potential bracket indentation styles.

Regardless, IMHO the biggest issue with the current api is mainly the fact that MultilineBlockBracketsOnSameColumn is factually incorrect when Stroustrup style is applied, so I think an adjustment is at least worth discussing due to the potential confusion it could cause.

[nojaf]

Hello, thank you for bringing this up.

the MultilineBlockBracketsOnSameColumn is technically inaccurate when using the Stroustrup style, as by definition the brackets are on different columns.

That is not entirely true, there are situations when stroupstrup isn't applicable and MultilineBlockBracketsOnSameColumn is used.
Sample

Stroustrup isn't documented I see. I would accept a PR for that but as we are already in beta I don't have any desire to change the public API.

Lastly, as the setting is forbidden fruit I can very much live with the very minor UX inconvennience.
Every setting is currently independent from the others. I have no interest in shady constructions if setting A then setting B no longer counts and whatnot.

[josh-degraw]

That is not entirely true, there are situations when stroupstrup isn't applicable and MultilineBlockBracketsOnSameColumn is used.
Sample

That's a fair counter example, although it's worth noting that that's an exception to the style rule. My main point with that was that it can be confusing to have to specify putting brackets on the same column when the main purpose of Stroustrup style is to have them on different columns, so having the effects of SameColumn apply by default would result in less confusion imho without changing any of the public API.

Every setting is currently independent from the others. I have no interest in shady constructions if setting A then setting B no longer counts and whatnot.

That's actually kind of my main point, that they're not currently independent. If both settings have to be checked for one to apply, one is very dependent on the other. What I'm suggesting is to make it so that turning on the Stroustrup setting works independently, whether or not you've enabled any other settings.

A better word choice on my part probably would have been to say that the effects of the same column rule (those that would still be applied such as your example above) should be implied and enabled if Stroustrup is checked, rather than only if both settings are enabled manually.

EDIT: Worth noting that this was actually a point you mentioned in the original stroustrup issue:

[ ] Investigate if fsharp_multiline_block_brackets_on_same_column should be implied as true automatically.

This is essentially the summation of what this issue is bringing up, and I'm just voting in the affirmative :)

[josh-degraw]

The more I think about it, the more I think it makes the most sense to use a single flag for bracket formatting, since although Stroustrup currently relies a lot on the Allman implementations under the hood, from an end user perspective, "bracket style" can be seen as a mutually exclusive decision between a specific set of options, which I think is why a bunch of folks have gotten confused by the fact that Stroustrup currently requires 2 settings.

In this case, it could be represented by something like:

type BracketStyle =
    | Classic
    | Allman
    | Stroustrup

My gut tells me that this might also make the implementation points a bit clearer and potentially simpler because of exhaustive type checking.

I recognize though that this would require a lot of plumbing changes, but I'm willing to toy around with it and see if it's feasible, if it's a direction you're willing to entertain.

[nojaf]

I'm willing to go there eventually but not in the short term.
There is still much ground to cover here, I think the entire Stroustrup story (style guide, implementation and documentation) should be very mature before we can move to a bracket-style setting.

Sidenote: We also deliberately didn't call Allman Allman style in the setting name.
fsharp_multiline_block_brackets_on_same_column was a vague common denominator to cover everything listed in the G-Research style guide.

[josh-degraw]

Sidenote: We also deliberately didn't call Allman Allman style in the setting name.
fsharp_multiline_block_brackets_on_same_column was a vague common denominator to cover everything listed in the G-Research style guide.

Fair enough, I actually agree since it's not "true" Allman (it could also be argued that it's not "true" Stroustrup either haha). But yeah I messed around with it a bit yesterday and I think for a real implementation I would do something like:

type MultilineBracketStyle = 
    | Classic 
    | Aligned 
    | ExperimentalStroustrup

For what it's worth, I had some time yesterday and I was able to get an implementation fully working, and I do think at least from my perspective it made it a lot easier to understand the implementation differences between the 3 bracket styles.

The only major task I still would need to do would be to add a bit of logic to keep the editorconfig parsing backwards compatible, but the actual implementation required fewer changes than I expected to get all the formatting tests to pass.

I think it can be done in a way that retains full backwards compatibility to avoid needing a breaking change, but still adds some tangible benefits to both the implementation and the end user sides especially in the form of clarity.

[nojaf]

Fix available in v5.2.0-beta-001