Implement a JsonConverterFactory equivalent of the existing default converter attribute #99
Conversation
| /// with a provider of <see cref="DateTimeZoneProviders.Tzdb"/>. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// This is a factory equalivalent of <see cref="NodaTimeDefaultJsonConverterAttribute"/>. |
| public override bool CanConvert(Type typeToConvert) => Converters.ContainsKey(typeToConvert); | ||
|
|
||
| /// <inheritdoc /> | ||
| public override JsonConverter CreateConverter(Type typeToConvert, JsonSerializerOptions options) => |
There was a problem hiding this comment.
Consider renaming options to unused so that it's obvious that the parameter is being deliberately ignored?
There was a problem hiding this comment.
Would prefer not to do that, as it can cause confusion with named arguments. But I could fill in documentation directly instead of inheriting it, and stress that the parameter is unused?
There was a problem hiding this comment.
Oh, good point about named arguments.
I think it's okay to leave the documentation as-is: it's still correct, it's just that the options aren't relevant to any of the returned converters (I assume that is true, rather than it being a bug that we're not looking at them).
If <inheritdoc/> allows adding extra information (like {@inheritDoc} in Java), you could add a sentence here, but I'm not sure it's necessary.
|
|
||
| /// <inheritdoc /> | ||
| public override JsonConverter CreateConverter(Type typeToConvert, JsonSerializerOptions options) => | ||
| Converters.TryGetValue(typeToConvert, out var converter) ? converter : null; |
There was a problem hiding this comment.
Perhaps factor this out as internal static JsonConverter? GetConverter(Type), then also call it from NodaTimeDefaultJsonConverterAttribute.CreateConverter() above, and make Converters here private?
You could also implement CanConvert(type) as GetConverter(type) != null, meaning that the only access to the dictionary is via that method. (Since the converters are singletons, that's basically the same cost.) Not sure whether that's worth doing, though.
There was a problem hiding this comment.
Yes, I think that makes a lot of sense. Will do so.
|
|
||
| namespace NodaTime.Serialization.Test.SystemTextJson; | ||
|
|
||
| public partial class NodaTimeDefaultJsonConverterFactoryTest |
There was a problem hiding this comment.
Perhaps give this a more-specific name. This isn't testing NodaTimeDefaultJsonConverterFactory per se, but the use of that with source generation.
You might also link to e.g. https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/source-generation in comments here to explain what's going on?
Separately, let's add a test (which could be called NodaTimeDefaultJsonConverterFactoryTest) to assert over the factory methods directly? I know we have some coverage in NodaTimeDefaultJsonConverterAttributeTest, but it might be nice to e.g. verify that CreateConverter() for each type we care about returns a non-null instance with correct Type property, etc, and that CanConvert() is consistent with those.
There was a problem hiding this comment.
I've left the source generation test present in the same class (but renamed it to SourceGenerationCompatibility) and added more "normal" tests above.
jskeet
force-pushed
the
converter-factory
branch
from
11b72af
to
e206e22
Compare
|
@malcolmr: I've pushed another couple of commits (after rebasing); PTAL. |
This refactors NodaTimeDefaultJsonConverterAttribute to use the converter dictionary which is now in NodaTimeDefaultJsonConverterFactory. Fixes nodatime#97.
…daConverterBase.CanConvert It looks like System.Text.Json doesn't need or expect a converter to advertise that it can handle nullable value types. (The default implementations don't do so.) It's entirely possible that we should actually just not override the method at all here, and only do so in NodaDateTimeZoneConverter, but I don't want to change the implementation right now.
jskeet
force-pushed
the
converter-factory
branch
from
e206e22
to
780449d
Compare
Fixes #97.
The only test is for source generation, as that's the use case for which this is being provided.
The first commit is just renaming; the second commit is actual code.