thoth-org / Thoth.Json

Library for working with JSON in a type safe manner, this libs is targeting Fable
https://thoth-org.github.io/Thoth.Json/
MIT License
152 stars 36 forks source link

Mutualise `encodeArray`, `encodeList`, `encodeSeq` #199

Closed MangelMaxime closed 3 months ago

MangelMaxime commented 3 months ago

In the encoder helpers we have several ways to encode a "sequence" of items.

https://github.com/thoth-org/Thoth.Json/blob/4f34bac2b4caa121a4c68aa3e3288cb231f3a9d0/packages/Thoth.Json.Core/Types.fs#L29-L31

Should we remove all of them except one ?

I suppose the only reason for keeping them separate would be to avoid a transformation phase when supported by the JSON backend. Would mapping them have a lot of impact on performance?

For example, Newtonsoft backend is implemented has which means we pass the "sequence" as is:

https://github.com/thoth-org/Thoth.Json/blob/4f34bac2b4caa121a4c68aa3e3288cb231f3a9d0/packages/Thoth.Json.Newtonsoft/Encode.fs#L27-L29

For other backend we do a mapping:

https://github.com/thoth-org/Thoth.Json/blob/4f34bac2b4caa121a4c68aa3e3288cb231f3a9d0/packages/Thoth.Json.Python/Encode.fs#L27-L33

@njlr What do you think about keeping only one of the member? If we keep only one which one should we keep?

I know that some people say that F# libraries should expose seq should it be this one? Or does it have a performance cost and we should favour ResizeArray or array?

njlr commented 3 months ago

I think it's better to have several functions.

  1. There are potential performance benefits that some back-ends may utilize
  2. Some F# users like to specify types in their by using specialized functions

e.g.

let foo xs = 
  xs 
  |> Array.map (fun x -> x + 1) // Makes xs an array
MangelMaxime commented 3 months ago
  1. There are potential performance benefits that some back-ends may utilize

Agree

2. Some F# users like to specify types in their by using specialized functions

I don't understand the relation as helpers are internal APIs.

But we will keep the separate APIs, does this means we should add a new one for ResizeArray too ? 🤔

njlr commented 3 months ago

I don't understand the relation as helpers are internal APIs.

Ah, I was responding to removing Encode.list, Encode.array, etc. in favor of just Encode.seq

But we will keep the separate APIs, does this means we should add a new one for ResizeArray too ? 🤔

I think this makes sense to add!

MangelMaxime commented 3 months ago

Ah, I was responding to removing Encode.list, Encode.array, etc. in favor of just Encode.seq

Ah yes, no I would still have exposed the different types for the Encode/Decode API