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

Conversation

kreathon
Copy link
Contributor

@kreathon kreathon commented Aug 15, 2024

Use TypeIs instead of TypeGuard

... to improve type narrowing for is_ok and is_err type guards.

Starting from Python 3.13 TypeIs is in the standards typing module. I decided to not introduce anything Python 3.13, since it should probably be handled in a seperate PR (see #181).

See: #193

Copy link
Member

@francium francium 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 PR. I can have a closer look at this, review the PEP 742 document, in the next few days

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.

@kreathon kreathon force-pushed the replace-typeguard-with-typeis branch from 194ab7f to 430f073 Compare August 17, 2024 15:42
@kreathon kreathon force-pushed the replace-typeguard-with-typeis branch from 5f9bb7d to 2fb4116 Compare August 18, 2024 07:51
@kreathon kreathon requested a review from francium August 18, 2024 07:52
@francium francium merged commit 0b855e1 into rustedpy:main Aug 18, 2024
5 checks passed
@francium
Copy link
Member

Thank you

@kreathon kreathon deleted the replace-typeguard-with-typeis branch August 18, 2024 17:50
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.

2 participants