Fix #151: do not wrap null with Some

[SamuelBerger]

Fixes #151

I have changed 2 existing tests because

• I think (with my limited knowledge in the subject) skipping a skippable field and explicitly setting it to null should have exactly the same effect
• they failed after my change 😝

type Person = { FirstName: string; LastName: string voption option }

{ "FirstName": "Marie" } and { "FirstName": "Marie", "LastName": null }

should both result to

{ FirstName = "Marie"; LastName = None } and not Some ValueNone (outer most Option should be None)

[GBirkel]

Hello! Can we get someone with write access to merge this? Looking forward to using it in our code...

[SamuelBerger]

Hi @Tarmil what is your opinion about the nested option problem?

• do you want to keep the current behaviour? (and in the PR the 2 test changes have to be reverted)
• do you think flattening nested options is fine? (in this case maybe the serialization side has to be double checked if it behaves consistently)

[Tarmil]

I would rather keep the current behaviour by default, and implement this as opt-in.

An example use case I have for deserializing null differently from the absence of the field is to parse HTTP PATCH requests. This is a situation where the JSON being deserialized represents a modification of a stored object, such that:

• A field "foo" = <something other than null> will set the value of foo;
• A field "foo" = null will remove foo from storage;
• An absent field "foo" will leave the existing foo as is.

But I also understand your use case, so a new option on JsonFSharpOptions is a welcome addition.

[SamuelBerger]

Yes, that makes sense. I will (hopefully this week) change the PR so it does not change the current behaviour and just fixes the bug.

The opt-in for flattening nested options would be a different change.

I also understand now the WithSkippableOptionFields configuration. It is a 1:1 replacement for https://github.com/Tarmil/FSharp.SystemTextJson/blob/master/docs/Format.md#skippable I initially understood it as "I won't be so harsh and throw an exception if you skip a field of type option instead of giving me a null" (as it was before 1.0 release)

[SamuelBerger]

@Tarmil one last thing to be on the safe side. Do you confirm #151 is a bug?

[SamuelBerger]

I assume it is not a bug then :) and close this tiny PR.