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.

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

Conversation

isaacabraham
Copy link

Just an initial WIP PR.

[<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

[<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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants