Fix: Add pickle support for UnwrapFailedError (#2048)#2049
Conversation
| - Add `default_error` parameter to `returns.converters.maybe_to_result`, | ||
| which provides a default error value for `Failure` | ||
|
|
||
| ### Bugfixes |
| # 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.
| # 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.
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.
| 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.
| 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 :) |
|
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