Skip to content

Conversation

@proboscis
Copy link
Contributor

I have made things!

Checklist

  • I have double checked that there are no unrelated changes in this pull request (old patches, accidental config files, etc)
  • I have created at least one test case for the changes I have made
  • I have updated the documentation for the changes I have made
  • I have added my changes to the CHANGELOG.md

Related issues

Fixes #2048

Description

This PR adds pickle support for UnwrapFailedError by implementing the __reduce__ method.
This allows the exception to be serialized/deserialized while preserving the halted_container information when using it in multiprocessing or other scenarios requiring serialization.

Changes

  • Added __reduce__ method to UnwrapFailedError class
  • Added tests to verify pickling works with both Maybe and Result containers
  • Updated CHANGELOG.md under Bugfixes section

Copy link
Member

@sobolevn sobolevn 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 for the quick PR!

- Add `default_error` parameter to `returns.converters.maybe_to_result`,
which provides a default error value for `Failure`

### Bugfixes
Copy link
Member

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

Comment on lines 13 to 21
# 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)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# 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)

Copy link
Member

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
Copy link

codecov bot commented Feb 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (82ef3ef) to head (d6ff6f7).
Report is 335 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@proboscis
Copy link
Contributor Author

Thank you for the quick review!

Comment on lines 10 to 12
error = None
serialized = None
error = None
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
assert error is not None

@proboscis
Copy link
Contributor Author

proboscis commented Feb 27, 2025

I apologize that my explanation of the issue was not clear.
Original problem about this pickling issue happend during unpickling time with this error:

TypeError: UnwrapFailedError.__init__() missing 1 required positional argument: 'container'

So my intention was to make sure this UnwrapFailedError is unpicklable by adding __reduce__.

Would keeping the deserialization line be appropriate?

@sobolevn
Copy link
Member

Yes, keeping unpickling / pickling is always fine

@proboscis
Copy link
Contributor Author

I hope the test is now in a good shape?
I also hope i am doing okey since this is my first time making a pr.

@sobolevn
Copy link
Member

I also hope i am doing okey since this is my first time making a pr.

Oh wow! Congrats! 🎉
I remember my first PR, it was really fun!

Thank you for you work and for contributing to my project, I really appreciate it :)

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

First PR merged 🎉

@sobolevn sobolevn merged commit e635d12 into dry-python:master Feb 27, 2025
24 checks passed
@proboscis
Copy link
Contributor Author

Thank you, I am so happy it got merged! :)
This library has been helping me so much, so thank you for making this great library!

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.

Add pickle support for UnwrapFailedError

2 participants