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

Change serde_json::from_value to use pass-by-reference #1128

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

rooney
Copy link

@rooney rooney commented May 1, 2024

If the function needs ownership, you should pass by value. If the function only needs a reference, you should pass by reference.

-- https://stackoverflow.com/a/64814149/342293

from_value doesn't need to own its parameter, so it should use pass-by-reference.

@GnomedDev
Copy link

Is this not an obvious breaking change that would require a serde_json 2.0?

@rooney
Copy link
Author

rooney commented Jun 6, 2024

Is this not an obvious breaking change that would require a serde_json 2.0?

Hmm yes, any other parts that should also been updated? (Apologies if these should be obvious, still learning)

This reverts commit 1eac9e3.
This preserves backward compatibility while allowing code that needs to keep ownership of the Value to pass is as reference.
@rooney
Copy link
Author

rooney commented Jun 21, 2024

@GnomedDev Redone the fix so now from_value accepts both &Value and Value (so no breaking changes, but maybe the latter can be deprecated?). New test for the &Value is on doctest (should I add more?)

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