-
Notifications
You must be signed in to change notification settings - Fork 77
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
Split result type into Ok and Err class - Updated #27
Conversation
👍 Had a quick look, it looks great so far. My only concern was losing the |
The problem is that
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. |
Alright I added a |
What about making the base class |
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 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. |
Existing 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.
Good point, hadn't thought of that. Defining ResultType in that way is easier and less error-prone.
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 |
I went ahead and implemented @Emerentius suggestion and removed the abstract base class. Instead we now have a
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't review the code yet, but here are some notes on API and README.
e5bbedc
to
688711e
Compare
For the data holder class approach, would it not cause confusion to the user should they try to create an
You lose consistency here, for example I think what we have now, potentially with a alias to wrap the What about a helper method like this?
|
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.
mypy doesn't understand this, unless you add a plugin |
@dbrgn Any chance you could take a look when you have time? :) |
@Jasonoro seems I overlooked your message. I'll try to take a look tonight! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@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. |
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). |
I think it's now ready to merge. I'll take a look tonight, and will merge if I don't see any blocker. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
2ef68c7
to
36434bb
Compare
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 Does that sound good for everybody? |
Just tried it out on an existing project. Ran fine without any modifications. Did convert over to use 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. |
I've been using my fork in a ~4 man project for a couple of weeks now. No-one has experienced any problems! |
Great, thanks for your feedback. I released 0.6.0rc1: https://pypi.org/project/result/0.6.0rc1/ |
This PR is a continuation of #18. Without @Emerentius this PR wouldn't have happened.
This is the implementation of #17:
Ok
andErr
Result.Ok()
andResult.Err()
class methodsOne 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?