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

feature/issue-175-auto #189

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

Conversation

njlr
Copy link
Contributor

@njlr njlr commented Apr 14, 2024

  • Introduce Auto module
  • Re-enable many tests

This PR introduces an Auto module that is agnostic to the JSON backend used. It does this by composing Encoders / Decoders from Thoth.Json.Core using reflection. There is some Fable specific stuff to pave over the limitation of reflection for some targets.

The majority of the old tests are re-enabled and unchanged.

LMK what you think!

@njlr
Copy link
Contributor Author

njlr commented Aug 6, 2024

Fixed merge conflicts

@MangelMaxime
Copy link
Contributor

Thank you for the update, I just got back from vacation will have a look to make a new Thoth.Json release this week and take this chance to look at the Auto API support once again.

@MangelMaxime
Copy link
Contributor

Hello @njlr,

I am finally starting to have a look at this PR seriously.

Looking at the code it seems like there are a lot of changes compared to the originals versions, do you remember what motivated these changes?

For example, you seems to use this pattern a lot:

#if FABLE_COMPILER
                let fail (innerType: Type) (x: string) : obj =
                    Decode.fail x |> box
#else
                let private failGenericMethodDefinition =
                    getGenericMethodDefinition "Fail"

                let fail (innerType: Type) (x: string) : obj =
                    failGenericMethodDefinition
                        .MakeGenericMethod([| innerType |])
                        .Invoke(null, [| x |])
#endif

instead of probably the DecoderCrate

    type private DecoderCrate<'T>(dec: Decoder<'T>) =
        inherit BoxedDecoder()
        override __.Decode(path, token) =
            match dec path token with
            | Ok v -> Ok(box v)
            | Error er -> Error er
        member __.UnboxedDecoder = dec

    let boxDecoder (d: Decoder<'T>): BoxedDecoder =
        DecoderCrate(d) :> BoxedDecoder

    let unboxDecoder<'T> (d: BoxedDecoder): Decoder<'T> =
        (d :?> DecoderCrate<'T>).UnboxedDecoder

Is it because it improves something over the crate?

@njlr
Copy link
Contributor Author

njlr commented Dec 30, 2024

Looking at the code it seems like there are a lot of changes compared to the originals versions, do you remember what motivated these changes?

Sorry, it has been a while!

IIRC, the thinking is to move all reflection and dynamic casts to the encoder/decoder generation step and out of the encode/decode stage. In theory this offers better performance for the most common use-case: generate an encoder/decoder once and then call it many times.

The Auto module uses reflection to invoke the core API in an automatic way, but does not contain any special encoder/decoder types.

@MangelMaxime
Copy link
Contributor

Ok thank you for the answer.

At least now, Thoth.Json has benchmark setup so it should be possible to make a quick comparaison between both approach an see which one works the best at least for .NET.

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