Use explicit subconverters for [NodaTimeDefaultJsonConverter] for Interval and DateInterval #136
Conversation
…erval and DateInterval Fixes nodatime#135
|
cc @kaydensigh to check whether this looks right to you |
There was a problem hiding this comment.
https://github.com/nodatime/nodatime.serialization/blob/e1f2cf4/src/NodaTime.Serialization.SystemTextJson/NodaConverters.cs#L52 also has a This must be used in a serializer which also has an instant converter. comment - is that still true after this change?
| @@ -40,7 +40,7 @@ private static Dictionary<Type, JsonConverter> CreateConverterDictionary() | |||
| Add(NodaConverters.AnnualDateConverter); | |||
| Add(NodaConverters.DurationConverterImpl); | |||
| Add(NodaConverters.InstantConverter); | |||
| Add(NodaConverters.IntervalConverter); | |||
| Add(new NodaIntervalConverter(NodaConverters.InstantConverter)); | |||
There was a problem hiding this comment.
I must admit that I'm not entirely following this change, but should this change be made to the NodaConverters.IntervalConverter property itself?
(That would also obviate the need for two separate constructors for NodaIntervalConverter etc, I think.)
There was a problem hiding this comment.
I don't think so - because if someone creates a set of options and specifies a non-default Instant converter (e.g. new NodaJsonSettings { IntervalConverter = ...}, that should be used for converting the instants in an interval, IMO.
| /// <summary> | ||
| /// Instant converter to use, overriding whatever is in JsonSerializerOptions. | ||
| /// </summary> | ||
| private readonly JsonConverter<Instant> instantConverter; |
There was a problem hiding this comment.
(I'm surprised this doesn't need to be declared as nullable; but also later I'm also asking if we need to accept null here.)
There was a problem hiding this comment.
We're not using nullable reference types in this project yet. (I should probably do so at some point.)
Yes, for the interval converter returned by that property. (Whereas now you can construct an interval converter that always uses a specific one.) |
Yup. As a user, having |
Fixes #135