-
-
Notifications
You must be signed in to change notification settings - Fork 142
Fix: Add pickle support for UnwrapFailedError (#2048) #2049
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
Fix: Add pickle support for UnwrapFailedError (#2048) #2049
Conversation
sobolevn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the quick PR!
| - Add `default_error` parameter to `returns.converters.maybe_to_result`, | ||
| which provides a default error value for `Failure` | ||
|
|
||
| ### Bugfixes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be 0.24.1
| # Serialize the error | ||
| serialized = pickle.dumps(error) | ||
|
|
||
| # Deserialize | ||
| deserialized_error = pickle.loads(serialized) # noqa: S301 | ||
|
|
||
| # Check that halted_container is preserved | ||
| assert deserialized_error.halted_container == Nothing | ||
| assert deserialized_error.halted_container != Some(None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # Serialize the error | |
| serialized = pickle.dumps(error) | |
| # Deserialize | |
| deserialized_error = pickle.loads(serialized) # noqa: S301 | |
| # Check that halted_container is preserved | |
| assert deserialized_error.halted_container == Nothing | |
| assert deserialized_error.halted_container != Some(None) | |
| # Serialize the error | |
| serialized = pickle.dumps(error) | |
| # Deserialize | |
| deserialized_error = pickle.loads(serialized) # noqa: S301 | |
| # Check that halted_container is preserved | |
| assert deserialized_error.halted_container == Nothing | |
| assert deserialized_error.halted_container != Some(None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, remove the testing logic from except
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2049 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 80 81 +1
Lines 2485 2575 +90
Branches 437 44 -393
==========================================
+ Hits 2485 2575 +90 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thank you for the quick review! |
| error = None | ||
| serialized = None | ||
| error = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| error = None | |
| serialized = None | |
| error = None | |
| serialized = None |
We only need this var :)
|
|
||
| # Deserialize | ||
| deserialized_error = pickle.loads(serialized) # noqa: S301 | ||
| assert error is not None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| assert error is not None |
|
I apologize that my explanation of the issue was not clear. So my intention was to make sure this UnwrapFailedError is unpicklable by adding Would keeping the deserialization line be appropriate? |
|
Yes, keeping unpickling / pickling is always fine |
|
I hope the test is now in a good shape? |
Oh wow! Congrats! 🎉 Thank you for you work and for contributing to my project, I really appreciate it :) |
sobolevn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First PR merged 🎉
|
Thank you, I am so happy it got merged! :) |
I have made things!
Checklist
CHANGELOG.mdRelated issues
Fixes #2048
Description
This PR adds pickle support for
UnwrapFailedErrorby implementing the__reduce__method.This allows the exception to be serialized/deserialized while preserving the
halted_containerinformation when using it in multiprocessing or other scenarios requiring serialization.Changes
__reduce__method toUnwrapFailedErrorclassMaybeandResultcontainers