search

SteveDunn / Vogen

A semi-opinionated library which is a source generator and a code analyser. It Source generates Value Objects
Apache License 2.0
877 stars 45 forks source link

Support `JsonSerializerOptions.NumberHandling` #493

Closed viceroypenguin closed 1 year ago

viceroypenguin commented 1 year ago

Describe the feature

The JsonConverter created for a ValueObject should follow the behavior specified by JsonSerializerOptions.NumberHandling, namely that if the value is set to JsonNumberHandling.AllowReadingFromString, then the converter should allow reading from a string token type as well as a number token type.

Note: you can assign this to me, I'll try to get a PR to you asap.

SteveDunn commented 1 year ago

Hi @viceroypenguin - many thanks for the suggestion and the PR! That makes perfect sense. I've assigned it to you and commented on the PR on how to reset the snapshot files used for testing the generated code

MGRatEJOT commented 6 months ago

Hello, first of all thanks for this great library!

Unfortunately, however there are a couple of issues with this particular feature:

  1. There is a bug, because the generated code for reading a value uses options.NumberHandling == global::System.Text.Json.Serialization.JsonNumberHandling.AllowReadingFromString to check if reading from strings is enabled. However JsonNumberHandling is a [Flags] enum and therefore this fails if multiple options are set (e.g. NumberHandling = JsonNumberHandling.AllowReadingFromString | JsonNumberHandling.WriteAsString). I guess the code should use options.NumberHandling.HasFlag(global::System.Text.Json.Serialization.JsonNumberHandling.AllowReadingFromString)

  2. The feature is incomplete (in my opinion), because other NumberHandling flags are ignored. I guess at least JsonNumberHandling.WriteAsString should get evaluated for writing as the counterpart to JsonNumberHandling.AllowReadingFromString.

  3. There is also JsonNumberHandling.AllowNamedFloatingPointLiterals, but I am not sure if you want to support that.

SteveDunn commented 5 months ago

@MGRatEJOT - thanks for spotting the issue. For ints, the serializer always uses WriteNumberValue, which doesn't use a string. Then, in deserializer, when reading, ignores the flag because the token type is always a number. So you are correct, for this to work fully, then the serializer needs to account for the options flag.

I've moved this to a bug: https://github.com/SteveDunn/Vogen/issues/595