Update config to accept a single option for multiline_bracket_style #2658
Conversation
src/Fantomas.Core/FormatConfig.fs
Outdated
|
|
||
| static member OfConfigString(cfgString: string) = | ||
| match cfgString with | ||
| | "classic" -> Some(box Classic) |
There was a problem hiding this comment.
I don't think you need to box here, EndOfLineStyle doesn't do it, MultilineFormatterType does.
Let's settle on not boxing here and always box in the EditorConfig.fs code.
src/Fantomas.Core/FormatConfig.fs
Outdated
| @@ -25,6 +25,24 @@ type MultilineFormatterType = | |||
| | "number_of_items" -> Some(box MultilineFormatterType.NumberOfItems) | |||
| | _ -> None | |||
|
|
|||
| type MultilineBracketStyle = | |||
| | Classic | |||
There was a problem hiding this comment.
We probably want to go with something other else for Classic.
I think Don call this Cramped style. Pico is another option I guess.
There was a problem hiding this comment.
I like cramped!
| @@ -79,8 +79,11 @@ module WriterModel = | |||
| let update maxPageWidth cmd m = | |||
| let doNewline m = | |||
| let m = { m with Indent = max m.Indent m.AtColumn } | |||
|
|
|||
There was a problem hiding this comment.
Please remove these additional newlines. You didn't touch this code.
src/Fantomas.Core/Context.fs
Outdated
| @@ -1027,7 +1043,14 @@ let sepSemi (ctx: Context) = | |||
| <| ctx | |||
|
|
|||
| let ifAlignBrackets f g = | |||
There was a problem hiding this comment.
Maybe rename this to ifAlignOrStroustupBrackets?
src/Fantomas.Core/CodePrinter2.fs
Outdated
| match ctx.Config.MultilineBracketStyle with | ||
| | Aligned | ||
| | ExperimentalStroustrup -> aligned | ||
| | Classic when (List.exists (fun (fieldNode: FieldNode) -> fieldNode.XmlDoc.IsSome) node.Fields) -> aligned |
There was a problem hiding this comment.
The original code was already a bit icky but could you extract List.exists (fun (fieldNode: FieldNode) -> fieldNode.XmlDoc.IsSome) node.Fields to some value? Call it anyFieldHasXmlDoc maybe.
josh-degraw
force-pushed
the
bracket-du
branch
from
348f94d to
1d4c5c2
Compare
FWIW, I was specifically talking about just moving |
There was a problem hiding this comment.
Very, very nice, I'm all in favor of this.
Just one thing:
Configuration.fsx needs a little bit of adjustment.
Well, I think there is value in having most of the config type in that namespace.
You can only update that once there is a version on NuGet with these changes. |
Uh, sorry. Nevermind... |
There was a problem hiding this comment.
Thanks @josh-degraw! I'm very happy to see movement on this front.
…2658) * Add BracketStyle DU, update everything except editorconfig parsing * Rename enum and config member * Update editorconfig parsing to remain backwards-compatible * Remove need for computed property on FormatConfig * Fix rebase issues * Rename some backcompat tests * Don't box in MultilineBracketStyle.OfConfigString * Rename Classic to Cramped * Remove ifStroustrup and ifStroustrupElse & revert unintended newlines * Rename ifAlignBrackets to ifAlignOrStroustrupBrackets * Refactor record multiline expression condition
Updates configuration to accept a single option for multiline bracket style. Names could be bike-shedded if necessary. I made sure to keep it backwards compatible, so existing editorconfig settings using either of the previously existing settings will be respected as before.
One other point I'm not in love with is that it currently requires
open Fantomas.Core.FormatConfigany time it's used. I think I'd rather just move the type directly into theFantomas.Corenamespace, but I just left it for now. If you're open to that I'll move it.Closes #2425