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.

Sign up for GitHub

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

[WIP] Add basic support for "Consistent" Pattern Matching #3085

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
56e10d6
Add configuration flag.
isaacabraham May 17, 2024
6187cf6
Fotmatting
isaacabraham May 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 22 additions & 2 deletions src/Fantomas.Core/FormatConfig.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,19 @@ type MultilineFormatterType =
| "number_of_items" -> Some MultilineFormatterType.NumberOfItems
| _ -> None

type PatternMatchStyle =
| LineSpecific
| Consistent
static member ToConfigString(cfg: PatternMatchStyle) =
match cfg with
| LineSpecific -> "line_specific"
| Consistent -> "consistent"
static member OfConfigString(cfgString: string) =
match cfgString with
| "line_specific" -> Some LineSpecific
| "consistent" -> Some Consistent
| _ -> None

type MultilineBracketStyle =
| Cramped
| Aligned
Expand Down Expand Up @@ -227,7 +240,13 @@ type FormatConfig =

[<Category("Convention")>]
[<DisplayName("Applies the Stroustrup style to the final (two) array or list argument(s) in a function application")>]
ExperimentalElmish: bool }
ExperimentalElmish: bool

[<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
Copy link
Contributor

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.

Copy link
Author

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.

Copy link
Author

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?

Copy link
Contributor

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!

Copy link
Author

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!

Copy link
Contributor

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
with your new config thing.

The build errors I noticed on the CI, are likely a result of 8.0.300 being installed on the agent.

Copy link
Author

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.

}

member x.IsStroustrupStyle = x.MultilineBracketStyle = Stroustrup

Expand Down Expand Up @@ -268,4 +287,5 @@ type FormatConfig =
MultilineBracketStyle = Cramped
KeepMaxNumberOfBlankLines = 100
NewlineBeforeMultilineComputationExpression = true
ExperimentalElmish = false }
ExperimentalElmish = false
ExperimentalPatternMatchStyle = LineSpecific }
19 changes: 19 additions & 0 deletions src/Fantomas.Tests/EditorConfigurationTests.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -543,3 +543,22 @@ fsharp_experimental_elmish = true
let config = EditorConfig.readConfiguration fsharpFile.FSharpFile

Assert.That(config.ExperimentalElmish, Is.True)

[<Test>]
let fsharp_consistent_pattern_matching_style () =
let rootDir = tempName ()

let editorConfig =
"""
[*.fs]
fsharp_experimental_pattern_match_style = consistent
"""

use configFixture =
new ConfigurationFile(defaultConfig, rootDir, content = editorConfig)

use fsharpFile = new FSharpFile(rootDir)

let config = EditorConfig.readConfiguration fsharpFile.FSharpFile

Assert.That(config.ExperimentalPatternMatchStyle, Is.EqualTo Consistent)
7 changes: 7 additions & 0 deletions src/Fantomas/EditorConfig.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ let (|Number|_|) (d: string) =
| true, d -> Some(box d)
| _ -> None

let (|PatternMatchStyle|_|) pms =
PatternMatchStyle.OfConfigString pms

let (|MultilineFormatterType|_|) mft =
MultilineFormatterType.OfConfigString mft

Expand All @@ -86,6 +89,7 @@ let parseOptionsFromEditorConfig
| true, Number n -> n
| true, Boolean b -> b
| true, MultilineFormatterType mft -> box mft
| true, PatternMatchStyle pms -> box pms
| true, EndOfLineStyle eol -> box eol
| true, BracketStyle bs -> box bs
| _ -> defaultValue)
Expand All @@ -105,6 +109,9 @@ let configToEditorConfig (config: FormatConfig) : string =
| :? MultilineFormatterType as mft ->
$"%s{toEditorConfigName recordField.PropertyName}=%s{MultilineFormatterType.ToConfigString mft}"
|> Some
| :? PatternMatchStyle as pms ->
$"%s{toEditorConfigName recordField.PropertyName}=%s{PatternMatchStyle.ToConfigString pms}"
|> Some
| :? EndOfLineStyle as eols ->
$"%s{toEditorConfigName recordField.PropertyName}=%s{EndOfLineStyle.ToConfigString eols}"
|> Some
Expand Down
Loading