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

Read some System.Text.Json options #2655

Merged
merged 7 commits into from
May 8, 2020
Merged

Conversation

Sinhk
Copy link
Contributor

@Sinhk Sinhk commented Jan 27, 2020

Get global string enum serialization and casing (#2652) from System.Text.Json options.

When testing this I noticed the GetService<IOptions>() always returns an IOptions, as long as MvcNewtonsoftJsonOptions exists. It didn't have to be registered with the service provider. So I changed some of the code that checks if it should load options for Newtonsoft.Json or System.Text.Json to look for a NewtonsoftJsonOutputFormatter instead.

@Sinhk
Copy link
Contributor Author

Sinhk commented Mar 19, 2020

@RicoSuter Finally got around to redoing this. Tried to keep only what’s necessary for reading the options

@RicoSuter
Copy link
Owner

Looks good. Is there a possibility to add unit tests for that somehow? Just to avoid regressions and have it tested.

@Sinhk
Copy link
Contributor Author

Sinhk commented Mar 20, 2020

@RicoSuter Added some tests, checking if the options are set.
I didn't find any test project targeting netcoreapp3 so I added a new one. Is that fine or should the tests rather be added to one of the existing projects?

@RicoSuter
Copy link
Owner

Sorry for my late reply, added two comments, what do you think?

@RicoSuter
Copy link
Owner

I think it would make sense to extract a ConvertJsonSettingsToNewtonsoftSettings method and move it to njsonschema.. then we just need to map as good as possible, eventually maybe with a custom contract resolver which reads system.text.json metadata.. i think this would be a good path forward and users of json schema would also benefit.. what do you think?

@Sinhk
Copy link
Contributor Author

Sinhk commented May 5, 2020

Sounds like a good plan. I'll try to make a PR for that tomorrow.

@RicoSuter
Copy link
Owner

RicoSuter commented May 5, 2020

Another thing: We should somehow turn off reading of newtonsoft attributes - in the newtonsoft settings 😀. See https://github.com/RicoSuter/NSwag/issues/2762

RicoSuter added a commit to RicoSuter/NJsonSchema that referenced this pull request May 7, 2020
@RicoSuter
Copy link
Owner

Working on this now in your branch.

RicoSuter added a commit to RicoSuter/NJsonSchema that referenced this pull request May 7, 2020
@RicoSuter
Copy link
Owner

Can we merge this?

@Sinhk
Copy link
Contributor Author

Sinhk commented May 7, 2020

Yes, looks good 🙂

@RicoSuter RicoSuter merged commit b2edb46 into RicoSuter:master May 8, 2020
RicoSuter added a commit to RicoSuter/NJsonSchema that referenced this pull request May 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants