Change design for improved type safety

[Emerentius]

I'm proposing that the Result class should be split into two classes class Ok(Generic[T]) and class Err(Generic[E]) and that Result be only a type alias for Union[Ok[T], Err[T]]

This has the advantage that you can use isinstance as a replacement for matching and gain improved type checkability.

For example, take this code

r: Result[int, str] = Ok(2)
if r.is_ok():
    reveal_type(r.value) # returns Union[int, str]

There is an unnecessary ambiguity in the type of value. You can use unwrap, but if you have a bug in your code, it's now a runtime error and a type checker can't help you find it.

On the other hand, with unions you can do this:

r: Result[int, str] = Ok(2) # == Union[Ok[int], Err[str]]
if isinstance(r, Ok):
    reveal_type(r.value) # returns int

I can make a PR showing what this would look like.

[dbrgn]

That sounds interesting 🙂

[Jasonoro]

@Emerentius @dbrgn Are you both still interested in continuing with #18? That MR is extremely useful to us and if need be I can put in some work to get that MR up-to-date.

[dbrgn]

I'm definitely interested, I'm just a bit tight on free time to review. Getting the PR up to date and doing a review (ideally with a migration / upgrade guide for affected users) would definitely help.

If it looks good, it could become the 0.6 release.

CC @francium

[Jasonoro]

Alright, I updated everything here. The good news is that all tests (That are still relevant) pass. The bad news is as follows:

$ mypy result/typetests.py
result\typetests.py:9:23: error: Cannot infer type argument 1 of "map_or" of "Err"
result\typetests.py:12:22: error: Cannot infer type argument 1 of "map_err" of "Ok"
result\typetests.py:12:45: error: Cannot infer type argument 1 of <list>
Found 3 errors in 1 file (checked 1 source file)

This apparently is a bug in MyPy. However seeing as the issue is from 2013, this won't be fixed soon.

There are workarounds but those are all manual fixes. The question for you is if you are fine with this issue, because it's quite a breaking (and unwelcome) change in the use of the different map methods, even if it's not our fault.

I'll write a migration guide and submit the PR once we've discussed above.

[Jasonoro]

Okay I figured out the problem. The typetest was still using if res1.is_ok() instead of the new way of using if isinstance(res1, Ok). With the new way the error doesn't happen so that's great! I'll probably make a migration guide soon and submit a PR.

[dbrgn]

@Jasonoro that's great!