-
-
Notifications
You must be signed in to change notification settings - Fork 194
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
[WIP] Add basic support for "Consistent" Pattern Matching #3085
base: main
Are you sure you want to change the base?
[WIP] Add basic support for "Consistent" Pattern Matching #3085
Conversation
src/Fantomas.Core/FormatConfig.fs
Outdated
[<Category("Indentation")>] | ||
[<DisplayName("How to format pattern match expression")>] | ||
[<Description("Possible options include each line judged separately (default), or consistent (either all single line or all multiline)")>] | ||
ExperimentalPatternMatchStyle : PatternMatchStyle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer a boolean setting for this. This would be much more consistent with most other Fantomas settings.
Some reasons:
- Typo's are less likely in editorconfig.
- Online tool doesn't need to be update.
- There are only two values here, most other DU's have three.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I had thought about that. The reason I put it as an enum was because the original feature request had 4 cases (these two, plus "always single" and "always multiline"). Happy to move to a bool, although it would be a little less readable i.e. instead of mode, it would be e.g. ConsistentPatternMatching = true / false
with the false case being "implicitly" the standard / existing behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nonetheless, it would be helpful to know why the current unit test is broken - any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate your enthusiasm, but at the moment, we don't have enough support to justify having four different cases for this. Right now, we're considering just a boolean setting as an alternative approach. The other options aren't really worth pursuing at this stage.
it would be helpful to know why the current unit test is broken - any ideas?
To give you a better answer, I'd need to see the test running on CI. My initial guess is that there might be an issue with your DU.
Thanks for understanding!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really enthusiasm - just was thinking of the right thing to do. I've no problem to convert to a boolean and will do.
I've fixed the formatting so that should get the CI working anyway so that we can see the test fail on CI (hopefully) - that might enable you to explain why the test is failing, which would be good to know in order to improve my understanding of the Fantomas codebase, enabling me to become a more active participant in the future!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed you did not update
fantomas/src/Fantomas/Daemon.fs
Lines 125 to 173 in 873d9d7
[<JsonRpcMethod(Methods.Configuration)>] | |
member _.Configuration() : string = | |
let settings = | |
Reflection.getRecordFields FormatConfig.Default | |
|> Array.toList | |
|> List.choose (fun (recordField, defaultValue) -> | |
let optionalField key value = | |
value |> Option.toList |> List.map (fun v -> key, Encode.string v) | |
let meta = | |
List.concat | |
[| optionalField "category" recordField.Category | |
optionalField "displayName" recordField.DisplayName | |
optionalField "description" recordField.Description |] | |
let type' = | |
match defaultValue with | |
| :? bool as b -> | |
Some( | |
Encode.object | |
[ yield "type", Encode.string "boolean" | |
yield "defaultValue", Encode.string (if b then "true" else "false") | |
yield! meta ] | |
) | |
| :? int as i -> | |
Some( | |
Encode.object | |
[ yield "type", Encode.string "number" | |
yield "defaultValue", Encode.string (string<int> i) | |
yield! meta ] | |
) | |
| :? MultilineFormatterType as m -> | |
Some( | |
Encode.object | |
[ yield "type", Encode.string "multilineFormatterType" | |
yield "defaultValue", Encode.string (MultilineFormatterType.ToConfigString m) | |
yield! meta ] | |
) | |
| :? EndOfLineStyle as e -> | |
Some( | |
Encode.object | |
[ yield "type", Encode.string "endOfLineStyle" | |
yield "defaultValue", Encode.string (EndOfLineStyle.ToConfigString e) | |
yield! meta ] | |
) | |
| _ -> None | |
type' |> Option.map (fun t -> toEditorConfigName recordField.PropertyName, t)) | |
|> Encode.object |
The build errors I noticed on the CI, are likely a result of 8.0.300
being installed on the agent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks for this - that's the bit I couldn't find.
Just an initial WIP PR.