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.

Sign up for GitHub

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

Jump to bottom

Change order of generic Result types to [T, E] #8

Merged
merged 4 commits into from
Feb 23, 2020
Merged

Change order of generic Result types to [T, E] #8

merged 4 commits into from
Feb 23, 2020

Conversation

dbrgn
Copy link
Member

@dbrgn dbrgn commented Jun 19, 2018

This will fix #7.

@coveralls

This comment has been minimized.

Sign in to view
dbrgn
dbrgn commented Jun 19, 2018
View reviewed changes
result/result.py Outdated Show resolved Hide resolved
result/result.py Show resolved Hide resolved
result/typetests.py Outdated Show resolved Hide resolved
@snorfalorpagus
Copy link
Contributor

The typetests.py needs to be added to Travis:

$ mypy result/typetests.py 
result/typetests.py:6:8: error: No overload variant of "Ok" of "Result" matches argument types [builtins.str]

@dbrgn
Copy link
Member Author

dbrgn commented Jun 19, 2018

@snorfalorpagus they already are: https://travis-ci.org/dbrgn/result/jobs/394025444#L538 You probably need to update mypy in your virtualenv. It's currently pinned to python/mypy@29889c8 which fixes some problems with classmethod overloads.

@francium
Copy link
Member

I can have a go at finished up this PR if no one else is going to. Please advise on how to best take this over, I'm not sure of the workflow on github in such a situation.

@tyehle
Copy link
Contributor

tyehle commented Feb 21, 2020 via email

@dbrgn
Copy link
Member Author

dbrgn commented Feb 21, 2020

Yes, definitely, this is the last blocker for the next release, I'd say!

I'll look through the PR today and will rebase it against master. I don't actually remember whether it even needs further work or whether I simply left it open for potential feedback and then forgot about it 🙂

Also, @francium, since you did some PR work in the last weeks, are you interested in becoming a co-maintainer? Unfortunately I have too many projects and can't give this library the attention it should have.

@francium
Copy link
Member

Also, @francium, since you did some PR work in the last weeks, are you interested in becoming a co-maintainer? Unfortunately I have too many projects and can't give this library the attention it should have.

@dbrgn Happy to help out day-to-day with bugs and smaller things. 👍

@dbrgn
Copy link
Member Author

dbrgn commented Feb 21, 2020

@francium invited you as collaborator!

@dbrgn dbrgn force-pushed the 7-change-result-type-order branch 2 times, most recently from 945ae2d to 9f036b0 Compare February 21, 2020 16:28
@dbrgn
Copy link
Member Author

dbrgn commented Feb 21, 2020

Hm, mypy doesn't like Python 3.4 anymore. I disabled type checking in CI for 3.4.

@dbrgn
Upgrade mypy to 0.720
8b55e8c 
This is the last version that can be installed on Python 3.4.

However, running mypy does not work on Python 3.4 anymore, so exclude
type checks in the Travis config.
@dbrgn
Copy link
Member Author

dbrgn commented Feb 21, 2020

Ok, tests pass now and the PR should be feature-complete.

@francium feel free to review, I can't request it from you yet as long as you haven't accepted the invite 🙂

@francium francium self-requested a review February 22, 2020 00:35
francium
francium reviewed Feb 22, 2020
View reviewed changes
CHANGELOG.md Show resolved Hide resolved
francium
francium approved these changes Feb 22, 2020
View reviewed changes
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.

Excluded the typetest.py file from test coverage reporting to fix coverage regression. Otherwise, it looks good to me.

@dbrgn
Copy link
Member Author

dbrgn commented Feb 23, 2020

Excluded the typetest.py file from test coverage reporting to fix coverage regression. Otherwise, it looks good to me.

Ah, good point! Regarding workflow, I would prefer if PR owners are the only ones that push to the feature branch, otherwise it can happen that changes are accidentally overwritten due to a force push (e.g. after a rebase).

Thanks for the review 🙂 Let's merge this.

@dbrgn dbrgn merged commit ab6dea3 into master Feb 23, 2020
@dbrgn dbrgn deleted the 7-change-result-type-order branch February 23, 2020 14:14
@dbrgn dbrgn mentioned this pull request Feb 23, 2020
Closed
@francium
Copy link
Member

francium commented Feb 23, 2020 •
edited
Loading

I would prefer if PR owners are the only ones that push to the feature branch

Makes sense. Sure thing. Was trying to be helpful, but I'll just leave a comment next time.

@dbrgn
Copy link
Member Author

dbrgn commented Feb 23, 2020

No worries 🙂 Different projects handle that differently.

@dbrgn dbrgn mentioned this pull request Feb 23, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snorfalorpagus snorfalorpagus snorfalorpagus left review comments

@francium francium francium approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Type annotations: Order of T and E
5 participants
@dbrgn @coveralls @snorfalorpagus @francium @tyehle