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

Deduplicate similar methods of ContentDeserializer and ContentRefDeserializer #2557

Closed
wants to merge 2 commits into from

Conversation

Mingun
Copy link
Contributor

@Mingun Mingun commented Aug 6, 2023

This PR moves helper functions that is implemented identically in both content deserializers to the Content itself, which would guarantee that they are always have the same implementation.

The last commit also express deserialize_float method in terms of deserialize_integer method. While that is good for readability, maybe compiler will not inline it and it can be slightly slower. If you do not like it, you can drop the last commit from that PR

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.

Thank you!

This looks good except for the "Merge unexpected() into invalid_type()" commit. I think those are intentionally separate. unexpected is a relatively big chunk of logic that shouldn't need to be re-instantiated downstream for all the different E types that Content is used with. It is supposed to be instantiated once in the serde crate.

…eserializer

Thus we can guarantee that the implementation the same for both deserializers
thus we can be sure that all integers also would be handled when floats are requested
@Mingun
Copy link
Contributor Author

Mingun commented Aug 7, 2023

Makes sense. I removed this commit

@Mingun
Copy link
Contributor Author

Mingun commented Aug 14, 2023

The correct way is completely remove any attempt to return error from the deserializer itself. Closed in preference to #2569

@Mingun Mingun closed this Aug 14, 2023
@Mingun Mingun deleted the content-refactoring branch August 14, 2023 13:30
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.

2 participants