Skip to content

Conversation

@Tarmil
Copy link
Owner

@Tarmil Tarmil commented Nov 27, 2022

This PR introduces a new fluent syntax to define the options of the converter.

// New syntax:
let options =
    JsonFSharpOptions.Default()
        .WithUnionAdjacentTag()
        .WithUnionNamedFields()
        .WithUnwrapOption(false)
        .WithUnionTagName("type")
        .ToJsonSerializerOptions()

// Current syntax equivalent:
let options = JsonSerializerOptions()
JsonFSharpConverter(
    (JsonUnionEncoding.AdjacentTag
    ||| JsonUnionEncoding.NamedFields)
    &&& ~~~JsonUnionEncoding.UnwrapOption,
    unionTagName = "type")
|> options.Converters.Add

New API:

  • Initializer methods:
    • .Default(), .NewtonsoftLike(), .ThothLike(), .FSharpLuLike() that create options with the corresponding JsonUnionEncoding.
  • Fluent option methods:
    • A method .WithUnionFoo(?bool) for every enum case JsonUnionEncoding.Foo;
    • A method .WithFoo(value) for every optional argument foo of the JsonFSharpConverter constructor.
  • Build methods:
    • .ToJsonSerializerOptions() creates a JsonSerializerOptions with an F# converter;
    • .AddToJsonSerializerOptions(options) adds the F# converter to an existing JsonSerializerOptions, useful to integrate with ASP.NET Core, Giraffe, or other libraries.

Advantages:

  • More uniform syntax for the various options, instead of having some passed as enum and others as optional arguments.
  • Better binary compatibility: adding an optional argument to the JsonFSharpConverter constructor is binary-breaking (see Congrats on 1.0, please avoid binary breaking changes 😊 #132), whereas adding a fluent method isn't.
  • Allows creating a JsonSerializerOptions in a single expression, without having to create it and then mutate it. Especially useful when creating global options in a module.
  • Easier to create a base of common options and then alter them slightly for overrides:
    let commonOptions =
        JsonFSharpOptions.Default()
            .WithUnionAdjacentTag()
            .WithUnionNamedFields()
    
    let options =
        commonOptions
            .WithOverrides(dict [
                typeof<MyCustomType>, commonOptions.WithUnionTagName("type")
                typeof<OtherCustomType>, commonOptions.WithUnionUnwrapSingleFieldCases()
            ])
            .ToJsonSerializerOptions()

Unresolved questions:

  • The fluent syntax looks very C#-ish, even though it works perfectly fine in F#. Should we also add a more F#-ish syntax with module functions?
    let options =
        JsonFSharpOptions.Default()
        |> JsonFSharpOptions.unionAdjacentTag
        |> JsonFSharpOptions.unionNamedFields true
        |> JsonFSharpOptions.unwrapOption false
        |> JsonFSharpOptions.unionTagName "type"
        |> JsonFSharpOptions.toJsonSerializerOptions
  • What about JsonFSharpConverterAttribute? Since it's an attribute, it can't be fluent. Perhaps it should have settable properties that do the same thing as corresponding fluent methods, eg:
    [<JsonFSharpConverter(UnionNamedFields = true, UnwrapOption = false)>]
    type Foo = // ...

@cmeeren
Copy link
Contributor

cmeeren commented Dec 31, 2022

  • The fluent syntax looks very C#-ish, even though it works perfectly fine in F#. Should we also add a more F#-ish syntax with module functions?

I'm a bit late to the party, but for the record, I don't think the "F# alternative" adds any value. Fluent syntax works great in F#. Indeed, object programing (using classes etc.) works great and is even recommended in the official F# coding guidelines (not OOO stuff like inheritance, but using interfaces and classes).

@Tarmil
Copy link
Owner Author

Tarmil commented Dec 31, 2022

@cmeeren Yes, I ended up not implementing it.

gdar91 pushed a commit to gdar91/FSharp.SystemTextJson that referenced this pull request Jan 1, 2023
@cmeeren
Copy link
Contributor

cmeeren commented Jan 24, 2023

Just to be safe, can you verify that the old-style

options.Converters.Add(
  JsonFSharpConverter(
    unionEncoding = (
      JsonUnionEncoding.Default
      ||| JsonUnionEncoding.ExternalTag
      ||| JsonUnionEncoding.NamedFields
      ||| JsonUnionEncoding.UnwrapFieldlessTags
      ||| JsonUnionEncoding.UnwrapSingleFieldCases
      ||| JsonUnionEncoding.UnionFieldNamesFromTypes),
    unionFieldNamingPolicy = JsonNamingPolicy.CamelCase
  )
)

is equivalent to the new-style

options.Converters.Add(
  JsonFSharpOptions
    .Default()
    .WithUnionExternalTag()
    .WithUnionNamedFields()
    .WithUnionUnwrapFieldlessTags()
    .WithUnionUnwrapSingleFieldCases()
    .WithUnionFieldNamesFromTypes()
    .WithUnionFieldNamingPolicy(JsonNamingPolicy.CamelCase)
  |> JsonFSharpConverter
)

?

@Tarmil
Copy link
Owner Author

Tarmil commented Jan 25, 2023

Yes, it's equivalent. Plus, this:

fsOptions.AddToJsonSerializerOptions(options)

is equivalent to:

JsonFSharpConverter(fsOptions)
|> options.Converters.Add

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.

3 participants