Split result type into Ok and Err class - Updated

[Jasonoro]

This PR is a continuation of #18. Without @Emerentius this PR wouldn't have happened.

This is the implementation of #17:

• Changed Result to a union type of Ok and Err
• Removed the Result.Ok() and Result.Err() class methods
• Added a migration guide from 0.5 to 0.6
• Updated the README with the new way to use the library.

One last question: In #18 there was talk of remove the implicit Ok() == Ok(True) conversion. Do we still want to do this or do we want to leave this for another PR?

[coveralls]

Coverage remained the same at 100.0% when pulling 2ef68c7 on Jasonoro:split_result_type into 2f7c55a on dbrgn:master.

[francium]

👍 Had a quick look, it looks great so far.

My only concern was losing the isinstance(res, Result). Regarding that, excuse me if this is a really silly suggestion for reasons I'm not seeing, but would it make sense to have Result be a abstract base class and have Ok, Err extend/implement that base class? I suppose then isinstance(res, Result) would work, no?

[Jasonoro]

The problem is that Result already is defined as Union[Ok[T], Err[E]], so we can't redefine it as a abstract base class. So we have 2 options:

1. Put 2 different Result types in 2 different namespaces. So as an example: a typing.Result type for return type annotations and a instance.Result type for using isinstance(res, instance.Result)

2. Use a different name for the abstract base class. So the return type of a function is Result[int, str] but if you want to use isinstance you use isinstance(res, ResultType).

I think option 2 is a lot better then option 1, as 1 will cause so much frustration when you import the wrong thing. It's just a bit counter-intuitive.

[Jasonoro]

Alright I added a ResultType class as an abstract base class of both Ok and Err. I also updated the migration guide and README, and added a test for it.

[francium]

What about making the base class Result and the type ResulType? isinstance(res, Result) looks better than isinstance(res, ResultType). Plus existing checks in previous version would continue to work too right?

[Emerentius]

We had this discussion already in the last PR and I don't believe adding a base class in is a good idea. It contains no type information about the contained generics. If it's just about having isinstance(res, ResultType) then you could also define ResultType = (Ok, Err). A base class can also be used in other ways like constructing instances or using it in type annotations, pretty much all of which are errors but won't be immediately flagged as such.

There's also the option of defining a holder-class like I laid out in #18 (comment) which allows modelling sum-types more faithfully, but also more verbosely.

[Jasonoro]

What about making the base class Result and the type ResulType? isinstance(res, Result) looks better than isinstance(res, ResultType). Plus existing checks in previous version would continue to work too right?

Existing isinstance checks would indeed work, but now you have to update all type annotations, since:

def test() -> Result[int, str]:
    return Ok(1)

needs to be updated to

def test() -> ResultType[int, str]:
    return Ok(1)

as we're switching the type names. I would prefer the current solution over this one, but if you disagree please say so.

We had this discussion already in the last PR and I don't believe adding a base class in is a good idea. It contains no type information about the contained generics. If it's just about having isinstance(res, ResultType) then you could also define ResultType = (Ok, Err). A base class can also be used in other ways like constructing instances or using it in type annotations, pretty much all of which are errors but won't be immediately flagged as such.

Good point, hadn't thought of that. Defining ResultType in that way is easier and less error-prone.

There's also the option of defining a holder-class like I laid out in #18 (comment) which allows modelling sum-types more faithfully, but also more verbosely.

I looked at that but then we are starting to get very verbose. I personally think that trade off isn't worth it, since allowing isinstance calls with ResultType = (Ok, Err) seems fine with me. However if anybody disagrees with that please bring it up!

[Jasonoro]

I went ahead and implemented @Emerentius suggestion and removed the abstract base class. Instead we now have a Result_ type that can be used in isinstance checks.

There's also the option of defining a holder-class like I laid out in #18 (comment) which allows modelling sum-types more faithfully, but also more verbosely.

We should really decide whether or not we want to implement this. Otherwise we'll be breaking backwards-compatibility twice when that change gets implemented. I'm not sure if I like the isinstance(var.member, Ok) way of checking the current type, but it's the most faithful way to represent the type. What do you think @francium?

[dbrgn]

I didn't review the code yet, but here are some notes on API and README.

[francium]

For the data holder class approach, would it not cause confusion to the user should they try to create an Ok directly? What's the expected creation interface? Is this what you had in mind (question for anyone here)?

r = Result.ok(5)

if not isinstance(r, Result):
  ...

if isinstance(r.member, Ok):
  ...
else:
  ...

You lose consistency here, for example isinstance(r, Result) vs isinstance(r.member, Ok). You're relying on the programmer to keep in mind that checking Result requires you to not have .member and checking for Ok/Err does.

I think what we have now, potentially with a alias to wrap the (Ok, Err), is fine. Of course, please take my opinion lightly, I've probably got the least amount of experience here with mypy and I don't use Python day to day so my Python senses are rusty (no pun intended) at the moment.

What about a helper method like this?

def isResultInstance(r) -> Bool:     # or isinstanceResult(r)
  return isinstance(r, (Ok, Err))

[Emerentius]

For the data holder class approach, would it not cause confusion to the user should they try to create an Ok directly? What's the expected creation interface? Is this what you had in mind (question for anyone here)? [...]

I'm skeptical it's worth it, but it would mimic the situation where you have one type for the collection of variants which you could define methods on as opposed to defining N methods for N variants. The latter just seems hacky and may run into more mypy edge cases due to using uncommon patterns.

What about a helper method like this?

def isResultInstance(r) -> Bool:     # or isinstanceResult(r)
  return isinstance(r, (Ok, Err))

mypy doesn't understand this, unless you add a plugin

[Jasonoro]

@dbrgn Any chance you could take a look when you have time? :)

[dbrgn]

@Jasonoro seems I overlooked your message. I'll try to take a look tonight!

[dbrgn]

Finally took the time to fully review this 🙂 I left a few last comments.

In other languages I'd probably design an abstract base class for the two types to ensure a consistent interface, but this is Python which favors duck typing. So I guess this is fine.

[Jasonoro]

@dbrgn Just missing your opinion on the last outstanding discussion.

I'm also not quite sure why the Travis-CI build is not running, it works fine on my fork but the status somehow doesn't seem to show up in the PR.

[dbrgn]

I'm also not quite sure why the Travis-CI build is not running, it works fine on my fork but the status somehow doesn't seem to show up in the PR.

I think as long as tests pass on your fork, we can ignore that for now. I'll probably migrate CI setup to GitHub actions anyways (the CI config is quite simple).

[dbrgn]

@dbrgn Just missing your opinion on the last outstanding discussion.

I think it's now ready to merge. I'll take a look tonight, and will merge if I don't see any blocker.

[dbrgn]

This looks good! Thank you so much for the work, @Jasonoro @Emerentius (and also @francium for reviewing).

I have since updated the CI: #28. I will now rebase your branch on top of master, to get the branch ready to merge.

Changes implemented

[dbrgn]

Awesome! 🎉 I squashed all commits into one and added all involved people as co-authors. I hope that's OK you all.

Before creating a new release, it would be great if at least one of you could install current master of result in an existing project to see if everything works out (just leave a comment in this PR). If it does, I will release a release candidate (due to the significant API changes), followed by a final release in 1-2 weeks.

Does that sound good for everybody?

[francium]

Just tried it out on an existing project. Ran fine without any modifications. Did convert over to use isinstance and that works as expected, mypy correctly narrowed down the type.

This is the code (...it's pretty crappy code) I tested against, if you wanted to know what I tested against. I didn't push the modifications, it was using 0.4.1, but even this will run fine on newest version, https://github.com/francium/sitegen/blob/master/sitegen.py.

[Jasonoro]

I've been using my fork in a ~4 man project for a couple of weeks now. No-one has experienced any problems!

[dbrgn]

Great, thanks for your feedback. I released 0.6.0rc1: https://pypi.org/project/result/0.6.0rc1/