Skip to content

Conversation

travis-leith
Copy link

fix for #40

@MangelMaxime
Copy link
Contributor

Sorry, for the delay the problem with this proposition of encoder is that the decoder explicitly say that a string cannot be null.

let inline isString (token: JsonValue) = not(isNull token) && token.Type = JTokenType.String

In this case, I think you should write custom coders called nullableString or something like to explicitly mention that it allows null strings.

Or shadow the Decode.string and Encode.string with your own implementation.

@MangelMaxime
Copy link
Contributor

I am closing but please feel free to discuss it further.

@MangelMaxime
Copy link
Contributor

Wait, I think I read the change wrong now that I see the linked issue to it.

I will re-think my decision sorry about that.

@MangelMaxime MangelMaxime reopened this Sep 11, 2021
@MangelMaxime
Copy link
Contributor

MangelMaxime commented Sep 12, 2021

Hello,

@travis-leith I am unable to produce failing test with the current version of Thoth.Json.Net.

Code I tried:

            testCase "a string with new line works" <| fun _ ->
                let expected = "\"a\\nb\""
                let actual =
                    Encode.string "a\nb"
                    |> Encode.toString 4

                equal expected actual

            testCase "a string with new line character works" <| fun _ ->
                let expected = "\"a\\\\nb\""
                let actual =
                    Encode.string "a\\nb"
                    |> Encode.toString 4

                equal expected actual

            testCase "a string with tab works" <| fun _ ->
                let expected = "\"a\\tb\""
                let actual =
                    Encode.string "a\tb"
                    |> Encode.toString 4

                equal expected actual

Can you please provide me with a reproduction code for your error?

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