Repository navigation
feat: add per-variant overloads to Option/Result methods - #111
mittalabhi2123 wants to merge 1 commit into
Conversation
…pital#98) Adds @overload pairs to Option.{map,flatten,and_then,ok_or,unwrap_or_else, and_,or_,or_else,ok_or_else,inspect,unzip,zip,zip_with,reduce,xor} and Result.{flatten,ok,err,unwrap_or_else,map_err,inspect,inspect_err,and_,or_, or_else} so return types narrow to the precise Some/Null or Ok/Err variant based on self's type, following the existing unwrap_or/is_some pattern. transpose is intentionally left un-overloaded on both types: pyright distributes overloads per-union-member when called on a widened Result[Option[T], E] (as an existing typing test does on purpose), which produces a correct but unsimplified union that breaks that test's assert_type. This needs its own follow-up. reduce's prior signature was typed as -> Option[R], which was actually wrong when only one side is Some (it returns T or O, not R); the new overloads correct this. Fixes two benchmarks that passed Option.map/and_then as bare first-class callables, which breaks once those methods are overloaded (pyright can't bind an overloaded method reference to a single callable signature). Adds typing-only check_* coverage in tests/test_typing/ for all new overloads, following the existing assert_type/isinstance convention.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
OutSquareCapital
left a comment
OutSquareCapital
left a comment
There was a problem hiding this comment.
The actual changes are looking great! Thank you for the work.
I left some comments on the tests and benchmarks changes
| return value * 2 | ||
|
|
||
| assert benchmark(Some(10).map, double) == Some(20) | ||
| assert benchmark(lambda: Some(10).map(double)) == Some(20) |
There was a problem hiding this comment.
I'd rather keep ParamSpec usage instead of lambdas when possible. Any particular reason for these changes?
| _b = assert_type(opt.inspect(print), Null[int]) | ||
|
|
||
|
|
||
| def check_option_unzip() -> None: |
There was a problem hiding this comment.
All three type checkers are failing here?
|
|
||
|
|
||
| def check_option_ok_or() -> None: | ||
| opt: Option[int] = Some(10) |
There was a problem hiding this comment.
We want to let type checker inference do it's job as much as possible. explicit annotations should be avoided, or have a clear motivation. Here it should be fine in any case.
If there's issues with Literal[10] vs int, Dog and Animal classes could be preferable IMO
There was a problem hiding this comment.
There's a lot of code repetition here.
Would be better to centralize as much as possible in pre-existing overload check function, or in new functions but without redundant assertions and divergent examples who swap [int, str] VS [int, int].
It's especially important since it's basically "dead code": doesn't test anything except type checkers behavior, hence we want to keep this as minimal as possible.
This branch has not been deployed
2 participants
Addresses #98