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

Provide a way to chain Err to Exception #93

Closed
ileixe opened this issue May 27, 2022 · 10 comments · Fixed by #95
Closed

Provide a way to chain Err to Exception #93

ileixe opened this issue May 27, 2022 · 10 comments · Fixed by #95
Assignees

Comments

@ileixe
Copy link
Contributor

ileixe commented May 27, 2022

Hi,

Thanks for a cool library. I've tried to find a solid way to manage error cases in Python like Rust and found this elegant library.

We're migrating some of our APIs using result and miss a feature to cover some common patterns we've found. As our library clients may not want to change their code base to be aware of result.Ok or result.Err (I hope so in the future though), we've decided to change our internal APIs only as a initial step.

So basically we have some of similar codes like

# Internal API
def _get_user() -> Result[User, str]:
    ...

def _raise(e):
    raise e

# Public API
def get_user():
    return _get_user().map_err(lambda e: _raise(UserNotFoundException)).unwrap()   

This code works but have several ugliness including

  • As lambda could not accept raise, we could not help to have a weird wrapper function like _raise
  • We broke map_err() signature as the function accept a callable to return Err value

So basically, I'd like to ask having such API like below

def get_user():
    # Return user when Ok otherwise raise UserNotFoundException which accepting error
    return _get_user().ok_or(UserNotFoundException)

I don't think ok_or() is a right API name to reflect the feature (and is different from ok_or() in Rust), so please understand the intention only. It seems opposite API of as_result() as it translates Exception to Result and now I'm asking a API for Result.err to Exception.

I think introducing Try API may be the right one to implement such feature but do not aware of the project's road map, so any viable solution would be appreciated.

Thanks!

@wbolster
Copy link
Member

something like .unwrap_or_raise() maybe? it's definitely in the ‘unwrap’ family.

but raising plain exceptions without any info (e.g. no message) are typically not nice APIs for consumers, so we should do better than that.

i think this would need variants to a) simply raise an exception without even looking at the Err value, and b) a way to convert the Err value into a custom exception instance, maybe with a callable (could be a lambda) or something like that

@ileixe
Copy link
Contributor Author

ileixe commented May 27, 2022

Yes, "unwrap" family seems to be a right one.

I agree plain Exceptions may have low value (and we actually have added more information into the Exception although I skipped in the code above), and option b is the actual one that we are looking for.

@ileixe
Copy link
Contributor Author

ileixe commented Jun 7, 2022

Rather than API itself, #95 has a issue which mypy could not detect signature due to the same issue (python/mypy#230).

Any suggestions to provide better API?

@francium
Copy link
Member

Hi guys, sorry been busy so haven't been involved.

@ileixe have you look at #17, this was mentioned before and seems to be linked already to python/mypy#230

@francium
Copy link
Member

@wbolster

the basics here look good 👍🏼 but the API feels not so nice.

Could you elaborate on what you don't like about the suggested API?

@ileixe
Copy link
Contributor Author

ileixe commented Jun 12, 2022

@francium Are you mentioning the workaround in the #17? If then, yes I tried it and saw it worked: Using cast() or make lambda function having type definition, but it looks ugly enough to wipe out type safe benefit.

@wbolster
Copy link
Member

💡 just brainstorming…

i don't quite like how this proposed api makes it almost inevitable to use a lambda construct for something that's supposed to do something simple. (fwiw, i don't like lambdas in general, in python at least.)

maybe this could handle Type[BaseException] in a special way? so that it can be either a callable (such as a lambda) or an exception class directly?

res.unwrap_or_raise(ValueError)  # raise ValueError()

however, nice errors, which this is about in the first place, typically require more than just an exception class, e.g. a message:

res.unwrap_or_raise(ValueError, "foo")  # raise ValueError("foo")

but sometimes it's not just a string, so maybe also this?

res.unwrap_or_raise(MyException, *args, **kwargs)  # raise MyException(*args, **kwargs)

… but these may be hard to type correctly.

passing an exception instance could work but would always construct it, even when it's not needed:

res.unwrap_or_raise(ValueError("foo"))  # hmmm :/

@ileixe
Copy link
Contributor Author

ileixe commented Jun 14, 2022

i don't quite like how this proposed api makes it almost inevitable to use a lambda construct for something that's supposed to do something simple. (fwiw, i don't like lambdas in general, in python at least.)

I agree.

so that it can be either a callable (such as a lambda) or an exception class directly?

I'm inclined to provide a single specific way. From my perspective, all I want to pass some information from Err to Exception which will be raised.

How about this?

def unwrap_or_raise(self, exception: BaseException) -> NoReturn:
     raise exception(self._value)

res = Err("My error message")
res.unwrap_or_raise(ValueError)

# This will be rendered as
# ValueError: my error message

To be more specific, I can

class CustomError:
    def __repr__(self):
        return "My error with information"


res = Err(CustomError())
res.unwrap_or_raise(ValueError)

# This will be rendered as
# ValueError: My error with information

@wbolster
Copy link
Member

the basic exception constructor signature (which most built-in exceptions inherit) allows any number of arguments, which are later accessible via the .args attribute:

class BaseException:
    args: tuple[Any, ...]
    ...
    def __init__(self, *args: object) -> None: ...

https://github.com/python/typeshed/blob/master/stdlib/builtins.pyi#L1771

most uses actually use only 1 argument, so perhaps .unwrap_or_raise(ValueError) is good enough after all?

@ileixe
Copy link
Contributor Author

ileixe commented Jun 17, 2022

@wbolster yes, i think so.

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

Successfully merging a pull request may close this issue.

3 participants