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

feat: add overload to unwrap_or_raise to raise the exception itself #192

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mokeyish
Copy link

@mokeyish mokeyish commented Jul 23, 2024

add overload to unwrap_or_raise to raise the exception itself

   n2 = Err(ValueError('nay'))
   with pytest.raises(ValueError) as exc_info:
        n2.unwrap_or_raise()

@wbolster
Copy link
Member

lacking a clear motivation for this change (hint hint), i'm not convinced how this is useful looking at the diff only. is the intention to make try/except blocks simpler?

some context:

.unwrap_or_raise() is intended to raise an instance of the specified exception class. this pull request changes that to make that optional, and raise whatever the Err value was, but only if it the error value is an exception.

for Err types that hold an exception instance, plain .unwrap() already attaches that exception instance to the raised UnwrapException, via its .__cause__ attribute:

>>> import result
>>> err = result.Err(ValueError("oops"))
>>> try:
...     err.unwrap()
... except result.UnwrapError as exc:
...     print(repr(exc))
...     print(repr(exc.__cause__))
... 
UnwrapError("Called `Result.unwrap()` on an `Err` value: ValueError('oops')")
ValueError('oops')

@mokeyish
Copy link
Author

I just want the ValueError, not the UnwrapError. due to external code doesn't know about UnwrapError.

Without this PR, I can only use err.unwrap_or_raise(lambda e: e), it's a bit troublesome.

@francium
Copy link
Member

francium commented Aug 8, 2024

@wbolster what about a new unwrap_or_raise_self method that does what @mokeyish is trying to do here. But leaving the existing unwrap_or_raise unchanged.

@wbolster
Copy link
Member

wbolster commented Aug 8, 2024

this PR is strictly an API addition that is backwards compatible. before, the argument was required:

def unwrap_or_raise(self, e: Type[TBE]) -> NoReturn:

after, the argument is optional:

    def unwrap_or_raise(self, e: Optional[Type[TBE]] = None) -> NoReturn:

the behaviour when passing an exception type (which was required before this PR) stays the same.

so i don't think this needs a new method at all. in fact, with the new info and reasoning, i think this change is fine as-is. wdyt @francium?

@wbolster wbolster self-requested a review August 8, 2024 13:13
@wbolster
Copy link
Member

wbolster commented Aug 8, 2024

fwiw, though the mentioned work-around may work, it's not type safe:

err.unwrap_or_raise(lambda e: e)

… b/c that lambda is not an exception type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants