Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add API entry for JsonDocument converter #60236

Merged
merged 6 commits into from
Apr 20, 2022

Conversation

strobelt
Copy link
Contributor

Fixes #59954 by adding an API entry for JsonDocument converter in the JsonMetadataServices

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 10, 2021
@ghost
Copy link

ghost commented Oct 10, 2021

Tagging subscribers to this area: @dotnet/area-system-text-json
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #59954 by adding an API entry for JsonDocument converter in the JsonMetadataServices

Author: strobelt
Assignees: -
Labels:

area-System.Text.Json

Milestone: -

Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think more work is needed to wire the converter into the source generator implementation. Could you add a few tests as well?

@eiriktsarpalis eiriktsarpalis self-assigned this Oct 11, 2021
@eiriktsarpalis eiriktsarpalis added this to the 7.0.0 milestone Oct 11, 2021
@layomia
Copy link
Contributor

layomia commented Oct 11, 2021

I think more work is needed to wire the converter into the source generator implementation. Could you add a few tests as well?

👍

Ideally, we want to abstract the JsonDocument serializer tests (and possibly the JsonElement tests) to be shared between the reflection and src-gen implementations of the serializer. You can use the ExtensionDataTests.cs-related diffs in #58912 as an example.

We also need to add JsonDocument as a "known" type in the source generator implementation - e.g,

AddTypeIfNotNull(_knownTypes, _jsonElementType);

@strobelt
Copy link
Contributor Author

I fixed the naming conventions and added JsonDocument to the src-gen implementation.

But I am having a very hard time creating the tests needed. I read the changes on the PR #58912 and from what I understood there were two main changes:

  • The System.Text.Json.SourceGeneration.Tests/Serialization/ExtensionDataTests.cs file was created with tests for the JsonExtensionDataAttribute
  • The System.Text.Json/tests/Common/ExtensionDataTests.cs file was moved to Common and was made dependent on the SerializerTests

If I understood correctly the idea was to abstract the JsonDocument tests from Serialization\JsonDocumentTests.cs and \Serialization\DomTests.cs into a structure similar to what we have in both the ExtensionDataTests files. Is this correct? And if so, could you help me do this correctly?

Sorry for not being of more help with this, and thanks for all the help!

@eiriktsarpalis
Copy link
Member

Hi @strobelt, we haven't had a chance to take a look at this yet. If you don't mind I will close this PR for now but we will revisit once we find the time to work on it.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 15, 2021
@eiriktsarpalis
Copy link
Member

I've rebased the changes and added a couple of simple unit tests. Rather than abstracting the existing JsonDocument/JsonElement suite, I included a couple of simple acceptance tests in the RealWorldContextTests class. We shouldn't need to replicate the full suite just to make sure we're wiring things correctly in the source generator.

@eiriktsarpalis
Copy link
Member

Test failures unrelated, cf. #68293

@eiriktsarpalis eiriktsarpalis merged commit 6ec5931 into dotnet:main Apr 20, 2022
@eiriktsarpalis
Copy link
Member

Thank you for your contribution @strobelt!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JsonSerializer support for JsonDocument types in source generation mode
3 participants