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

Complete support for formats treating nulls as None #1685

Closed
wants to merge 4 commits into from

Conversation

H2CO3
Copy link
Contributor

@H2CO3 H2CO3 commented Nov 29, 2019

This is a follow-up to my previous PR about being able to treat null as None. I've noticed a couple more cases where an internally-tagged enum with a struct variant containing () wasn't able to be deserialized. This PR amends them.

@H2CO3
Copy link
Contributor Author

H2CO3 commented Nov 29, 2019

Please feel free to squash the commits into the first one, my whack-a-mole approach to updating the tests doesn't contain any interesting information.

@H2CO3
Copy link
Contributor Author

H2CO3 commented Dec 18, 2019

@dtolnay Can you please take a look at this? Thanks!

@H2CO3
Copy link
Contributor Author

H2CO3 commented Feb 10, 2020

@dtolnay Sorry to bother again, but is there some progress around this PR?

@H2CO3 H2CO3 requested a review from dtolnay February 13, 2020 16:46
@H2CO3
Copy link
Contributor Author

H2CO3 commented Apr 5, 2020

Hey @dtolnay I noticed there was a new release a couple days ago… could you please look at this?

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

This is a high risk–low benefit change so I have not been able to prioritize reviewing it.

This breaks support for formats that distinguish units and options and do not treat them as equivalent. I understand that your format considers them the same thing by treating null as visit_none in the deserializer, but it's not clear to me that one behavior is necessarily universally better than the other to be worth breaking one in favor of the other.

@H2CO3
Copy link
Contributor Author

H2CO3 commented Apr 5, 2020

Ah, alright – I understand that, thanks.

@H2CO3 H2CO3 closed this Apr 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants