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

Restore section that was deleted #22514

Closed
wants to merge 4 commits into from

Conversation

tdykstra
Copy link
Contributor

@tdykstra tdykstra commented Jan 26, 2021

@tdykstra tdykstra requested a review from jozkee January 26, 2021 19:00
@tdykstra tdykstra marked this pull request as ready for review January 26, 2021 19:08
@jozkee
Copy link
Member

jozkee commented Jan 26, 2021

I wonder if this section was intentionally removed given that JsonSerializerOptions.IgnoreNullValues will become obsolete.

EDIT: I think I found the reason it was removed, see #21108 (comment).

@layomia @eiriktsarpalis Could dotnet/runtime#40099 be a good reason to avoid obsoletion?

@tdykstra tdykstra marked this pull request as draft January 26, 2021 20:37
@tdykstra
Copy link
Contributor Author

I think I found the reason it was removed, see #21108 (comment).

Yes that's it, thanks. I'll leave this PR open until your question about obsoletion is resolved, but I'll change it back to draft.

@tdykstra tdykstra changed the title Restore section that was accidentally deleted Restore section that was deleted Jan 26, 2021
@layomia
Copy link
Contributor

layomia commented Jan 26, 2021

@jozkee hmm I don't think the edge case of avoiding null overwriting instantiated properties on deserialization for the ignore-reference feature is worth keeping the IgnoreNullProperty alive (non-obsolete).

I just don't like the idea of having two advertised ways of doing the same thing (IgnoreNullValues and DefaultIgnoreCondition). If it is important to support the above scenario, then we can consider extending JsonIgnoreCondition to include values that affect deserialization.


@tdykstra, what motivated opening this PR? Just want to be sure I'm not missing any info, otherwise, I say we close this PR.

@tdykstra
Copy link
Contributor Author

I'm fine with closing it. @jozkee asked me where the section went, and I forgot the story behind the deletion so I investigated, found it was removed in the 5.0 PR, thought I had removed it accidentally, and rushed (rather too quickly, it turns out) to put it back.

@tdykstra tdykstra closed this Jan 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants