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

Replace TypeGuard with TypeIs #194

Merged
merged 3 commits into from
Aug 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ Possible log types:

## [Unreleased]

- `[changed]` Improve type narrowing for `is_ok` and `is_err` type guards by
replacing `typing.TypeGuard` with `typing.TypeIs` (#193)

## [0.17.0] - 2024-06-02

- `[added]` Add `inspect()` and `inspect_err()` methods (#185)
Expand Down
93 changes: 45 additions & 48 deletions README.md
Copy link
Member

Choose a reason for hiding this comment

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

Would you kindly revisit this README and update any other references to is_ok/is_err/isinstance so they're update to date and accurate with the changes being made here.

There's a bug/workaround mentioned in the FAQ section near the bottom, would you be able to while you're here verify it that needs any updating with the changes you've made here.

Finally, I think it may still be good to mention isinstance can be used, in the README, but is_ok/is_err may work better now(?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Finally, I think it may still be good to mention isinstance can be used, in the README, but is_ok/is_err may work better now(?)

So currently there are three options:

user_result: Result[int, str]

# Option 1:
if isinstance(user_result, Ok):
    reveal_type(user_result)  # "result.result.Ok[builtins.int]"
else:
    reveal_type(user_result)  # "result.result.Err[builtins.str]"

# Option 2A:
if is_ok(user_result):
    reveal_type(user_result)  # "result.result.Ok[builtins.int]"
elif is_err(user_result):
    reveal_type(user_result)  # "result.result.Err[builtins.str]"

# Option 2B: (without elif and is_err()): 
if is_ok(user_result):
    reveal_type(user_result)  # "result.result.Ok[builtins.int]"
else:
    reveal_type(user_result)  # "Union[result.result.Ok[builtins.int], result.result.Err[builtins.str]]"

# Option 3:
if user_result.is_ok():
    reveal_type(user_result)  # "Union[result.result.Ok[builtins.int], result.result.Err[builtins.str]]"
else:
    reveal_type(user_result)  # "Union[result.result.Ok[builtins.int], result.result.Err[builtins.str]]"

After this change "Option 2 A" is no longer required and we get a simplified "Option 2":

# Option 2:
if is_ok(user_result):
    reveal_type(user_result)  # "result.result.Ok[builtins.int]"
else:
    reveal_type(user_result)  # "result.result.Err[builtins.str]"

From a type inference / narrowing perspective "Option 1" and "Option 2" should be interchangeably (TypeIs is basically implemented with the isinstance code).

So the question is if "Option 1" or "Option 2" should be recommended / prefered? Or should the docs recommend neither of them?

For me personally I see:

  • is_ok() is shorter and more readable
  • isinstance does not rely on another import (but maybe Ok or Err do need to be imported additionally)

What do you think? @francium
What do others think?

There's a bug/workaround mentioned in the FAQ section near the bottom, would you be able to while you're here verify it that needs any updating with the changes you've made here.

This should not be related to the changes I made.

Would you kindly revisit this README and update any other references to is_ok/is_err/isinstance so they're update to date and accurate with the changes being made here.

I will do (after the discussion about isinstance vs. is_ok.

Copy link
Member

Choose a reason for hiding this comment

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

So the question is if "Option 1" or "Option 2" should be recommended / prefered? Or should the docs recommend neither of them?

I would think mentioning Option 1, but suggesting/hinting you can achieve more readable code using Option 2, may be a good approach? I'll leave it up to you, but I think at least briefly mentioning Option 1 is beneficial for discoverability.

Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,10 @@ def get_user_by_email(email: str) -> Result[User, str]:
return Ok(user)

user_result = get_user_by_email(email)
if isinstance(user_result, Ok): # or `is_ok(user_result)`
if is_ok(user_result):
# type(user_result.ok_value) == User
do_something(user_result.ok_value)
else: # or `elif is_err(user_result)`
else:
# type(user_result.err_value) == str
raise RuntimeError('Could not fetch user: %s' % user_result.err_value)
```
Expand Down Expand Up @@ -97,12 +97,9 @@ for a, b in values:

Not all methods
(<https://doc.rust-lang.org/std/result/enum.Result.html>) have been
implemented, only the ones that make sense in the Python context. By
using `isinstance` to check for `Ok` or `Err` you get type safe access
to the contained value when using [MyPy](https://mypy.readthedocs.io/)
to typecheck your code. All of this in a package allowing easier
handling of values that can be OK or not, without resorting to custom
exceptions.
implemented, only the ones that make sense in the Python context.
All of this in a package allowing easier handling of values that can
be OK or not, without resorting to custom exceptions.

## API

Expand All @@ -117,26 +114,29 @@ Creating an instance:
>>> res2 = Err('nay')
```

Checking whether a result is `Ok` or `Err`. You can either use `is_ok`
and `is_err` type guard **functions** or `isinstance`. This way you get
type safe access that can be checked with MyPy. The `is_ok()` or
`is_err()` **methods** can be used if you don't need the type safety
with MyPy:
Checking whether a result is `Ok` or `Err`:

``` python
>>> res = Ok('yay')
>>> isinstance(res, Ok)
True
>>> is_ok(res)
True
>>> isinstance(res, Err)
False
>>> is_err(res)
False
>>> res.is_ok()
True
>>> res.is_err()
False
if is_err(result):
raise RuntimeError(result.err_value)
do_something(result.ok_value)
```
or
``` python
if is_ok(result):
do_something(result.ok_value)
else:
raise RuntimeError(result.err_value)
```

Alternatively, `isinstance` can be used (interchangeably to type guard functions
`is_ok` and `is_err`). However, relying on `isinstance` may result in code that
is slightly less readable and less concise:

``` python
if isinstance(result, Err):
raise RuntimeError(result.err_value)
do_something(result.ok_value)
```

You can also check if an object is `Ok` or `Err` by using the `OkErr`
Expand All @@ -156,27 +156,6 @@ False
True
```

The benefit of `isinstance` is better type checking that type guards currently
do not offer,

```python
res1: Result[int, str] = some_result()
if isinstance(res1, Err):
print("Error...:", res1.err_value) # res1 is narrowed to an Err
return
res1.ok()

res2: Result[int, str] = some_result()
if res1.is_err():
print("Error...:", res2.err_value) # res1 is NOT narrowed to an Err here
return
res1.ok()
```

There is a proposed [PEP 724 – Stricter Type Guards](https://peps.python.org/pep-0724/)
which may allow the `is_ok` and `is_err` type guards to work as expected in
future versions of Python.

Convert a `Result` to the value or `None`:

``` python
Expand Down Expand Up @@ -358,7 +337,7 @@ x = third_party.do_something(...) # could raise; who knows?
safe_do_something = as_result(Exception)(third_party.do_something)

res = safe_do_something(...) # Ok(...) or Err(...)
if isinstance(res, Ok):
if is_ok(res):
print(res.ok_value)
```

Expand Down Expand Up @@ -468,6 +447,24 @@ from the non-unix shell you're using on Windows.

## FAQ

- **Why should I use the `is_ok` (`is_err`) type guard function over the `is_ok` (`is_err`) method?**

As you can see in the following example, MyPy can only narrow the type correctly
while using the type guard **functions**:
```python
result: Result[int, str]

if is_ok(result):
reveal_type(result) # "result.result.Ok[builtins.int]"
else:
reveal_type(result) # "result.result.Err[builtins.str]"

if result.is_ok():
reveal_type(result) # "Union[result.result.Ok[builtins.int], result.result.Err[builtins.str]]"
else:
reveal_type(result) # "Union[result.result.Ok[builtins.int], result.result.Err[builtins.str]]"
```

- **Why do I get the "Cannot infer type argument" error with MyPy?**

There is [a bug in MyPy](https://github.com/python/mypy/issues/230)
Expand Down
4 changes: 2 additions & 2 deletions docs/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@
- [`result.as_result`](./result.md#function-as_result): Make a decorator to turn a function into one that returns a ``Result``.
- [`result.do`](./result.md#function-do): Do notation for Result (syntactic sugar for sequence of `and_then()` calls).
- [`result.do_async`](./result.md#function-do_async): Async version of do. Example:
- [`result.is_err`](./result.md#function-is_err): A typeguard to check if a result is an Err
- [`result.is_ok`](./result.md#function-is_ok): A typeguard to check if a result is an Ok
- [`result.is_err`](./result.md#function-is_err): A type guard to check if a result is an Err
- [`result.is_ok`](./result.md#function-is_ok): A type guard to check if a result is an Ok


---
Expand Down
Loading
Loading