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.

Sign up for GitHub

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

Replace TypeGuard with TypeIs #194

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f20e323
Replace TypeGuard with TypeIs
kreathon Aug 15, 2024
430f073
Proposal: Mention type guard functons before isinstance
kreathon Aug 17, 2024
2fb4116
Update README.md
kreathon Aug 18, 2024
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
View file
Open in desktop
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
21 changes: 0 additions & 21 deletions README.md
View file
Open in desktop
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 @@ -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
4 changes: 2 additions & 2 deletions docs/README.md
View file
Open in desktop
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
128 changes: 64 additions & 64 deletions docs/result.md
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion setup.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ classifiers =
[options]
include_package_data = True
install_requires =
typing_extensions;python_version<'3.10'
typing_extensions>=4.10.0;python_version<'3.13'
package_dir =
=src
packages = find:
Expand Down
14 changes: 8 additions & 6 deletions src/result/result.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,12 @@
Union,
)

from typing_extensions import TypeIs

if sys.version_info >= (3, 10):
from typing import ParamSpec, TypeAlias, TypeGuard
from typing import ParamSpec, TypeAlias
else:
from typing_extensions import ParamSpec, TypeAlias, TypeGuard
from typing_extensions import ParamSpec, TypeAlias


T = TypeVar("T", covariant=True) # Success type
Expand Down Expand Up @@ -527,8 +529,8 @@ async def async_wrapper(*args: P.args, **kwargs: P.kwargs) -> Result[R, TBE]:
return decorator


def is_ok(result: Result[T, E]) -> TypeGuard[Ok[T]]:
"""A typeguard to check if a result is an Ok
def is_ok(result: Result[T, E]) -> TypeIs[Ok[T]]:
"""A type guard to check if a result is an Ok

Usage:

Expand All @@ -544,8 +546,8 @@ def is_ok(result: Result[T, E]) -> TypeGuard[Ok[T]]:
return result.is_ok()


def is_err(result: Result[T, E]) -> TypeGuard[Err[E]]:
"""A typeguard to check if a result is an Err
def is_err(result: Result[T, E]) -> TypeIs[Err[E]]:
"""A type guard to check if a result is an Err

Usage:

Expand Down
21 changes: 13 additions & 8 deletions tests/type_checking/test_result.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,16 @@
- case: map_result
disable_cache: false
main: |
from result import Ok, Err, is_ok, is_err

result = Ok(1)
err = Err("error")
if is_ok(result):
reveal_type(result) # N: Revealed type is "result.result.Ok[builtins.int]"
elif is_err(err):
reveal_type(err) # N: Revealed type is "result.result.Err[builtins.str]"
from result import Result, Ok, Err, is_ok, is_err

res1: Result[int, str] = Ok(1)
if is_ok(res1):
reveal_type(res1) # N: Revealed type is "result.result.Ok[builtins.int]"
else:
reveal_type(res1) # N: Revealed type is "result.result.Err[builtins.str]"

res2: Result[int, str] = Err("error")
if is_err(res2):
reveal_type(res2) # N: Revealed type is "result.result.Err[builtins.str]"
else:
reveal_type(res2) # N: Revealed type is "result.result.Ok[builtins.int]"
Loading