search

softdevteam / mattermost-github-integration

GitHub integration for Mattermost
Other
79 stars 50 forks source link

Port to python3 #33

Closed cormier closed 7 years ago

cormier commented 7 years ago

Hi! Thanks for working on this nice project.

Since python2.7 is reaching end of line, I thought it could be useful to port the project to python3.

fix #24

ptersilie commented 7 years ago

Thanks for the PR. Do we really need all of those requirements in requirements.txt? I think flask, requests, and pillow should be enough.

cormier commented 7 years ago

This requirements.txt is the output of pip freeze. The extra dependencies are the dependencies of flask, requests and pillow. It is not necessary to explicitely define them in requirements.txt because pip will install them anyway when installing the "main" dependencies of the project. However, by doing so, you cannot guarantee what version of the sub dependency pip will fetch.

I think that by formally defining all the dependencies in requirements.txt this project will be easier to maintain because the users will be able to reproduce the environment on which we test, thus limiting the chances of them experiencing issues on a combination of sub-dependencies we did not test.

ptersilie commented 7 years ago

Ah that makes sense. Good point.

ptersilie commented 7 years ago

I'm looking into this now. I will have to do some tweaking as I don't want to break support for Python2.

ptersilie commented 7 years ago

Pushed a change. The reason for this is that I don't want to break other peoples setup when they pull the recent changes and suddenly python2 doesn't work anymore, and they also likely have to reinstall the dependencies for python3.

What do you think?

cormier commented 7 years ago

I think that's fair. I'll be sure to integrate python2 support in the tests PR.

cormier commented 7 years ago

Just thought of this: for python 2 support, instead of handling urllib /urllib2 import, would it be simpler to just use requests since it's already in the dependencies?

ptersilie commented 7 years ago

Ah yes. That's a good idea. Then we also won't need that try/except block anymore, I presume?

cormier commented 7 years ago

Yes, exactly, because requests will do this for us.

https://github.com/kennethreitz/requests/blob/216aee441def9564698c3c186a4fcda76c218ad8/requests/compat.py#L39

ptersilie commented 7 years ago

Brilliant! That's much better of course.

cormier commented 7 years ago

I had a moment to work on this so here's a commit that implements the feature with requests

ptersilie commented 7 years ago

Cool thanks a lot. I didn't get around to merge this yet but will hopefully be able to do this soon after I an some tests.

ptersilie commented 7 years ago

Alright, this looks good now. I edited the Readme a bit to clarify that Python 2 and 3 both work and also put the dependencies back in as well so that people know what is needed and what is optional (and why).

If you are happy with this, I will squash and merge and then look at the other two PRs.

Thanks again for your work. It's much appreciated.

cormier commented 7 years ago

Looks good to me! I will rebase the other PRs on top of this one.