Made module exclusively Python3#14
Conversation
|
This branch should now cleanly merge onto master. It removes Python 2 runtimes from tox, and Travis. |
alexchamberlain
left a comment
alexchamberlain
left a comment
There was a problem hiding this comment.
Overall, this LGTM - a few comments on the boundaries.
github_webhook/__init__.py
Outdated
|
|
||
| import sys | ||
| if sys.version_info[0] < 3: | ||
| raise Exception( |
There was a problem hiding this comment.
Consider RuntimeError.
setup.py
Outdated
| license='Apache 2.0', | ||
| packages=["github_webhook"], | ||
| install_requires=['flask', 'six'], | ||
| install_requires=['flask==1.0.2'], |
There was a problem hiding this comment.
Consider >= 1.0.2.
|
|
||
| for hook in self._hooks.get(event_type, []): | ||
| hook(data) | ||
| resp = hook(data) |
There was a problem hiding this comment.
If the use case is sending an error, would this be better represented with an exception?
There was a problem hiding this comment.
In this case we're returning the http code that the wrapped function returns. This could be any http code such as a 301 which is not necessarily an error. I think the current approach is the best choice here, but I'm happy to be corrected.
setup.py
Outdated
| packages=["github_webhook"], | ||
| install_requires=['flask==1.0.2'], | ||
| tests_require=['mock', 'nose'], | ||
| tests_require=['mock', 'pytest', 'nose'], |
There was a problem hiding this comment.
We should remove mock and nose here.
tox.ini
Outdated
| flask | ||
| six | ||
| py{27,py}: mock | ||
| pypy: mock |
There was a problem hiding this comment.
If we're Python 3 only, is pypy appropriate?
There was a problem hiding this comment.
I've changed it to pypy3, but is there any real reason to support pypy?
tox.ini
Outdated
| pytest-cov | ||
| flask | ||
| pypy: mock | ||
| mock |
There was a problem hiding this comment.
We can remove mock.
This change uses the post 1.0 version of the Flask API. We drop support for Python 2 which allows us to remove dependencies on `mock` and `six`. Furthermore, we test against the documented `postevent` from GitHub. We choose to use the entire object rather than a `postevent`-like object in order to test against "real world" data. Signed-off-by: Aidan Delaney <adelaney21@bloomberg.net>
Changed the error thrown on detecting a legacy Python 2 environment to be a RuntimeError. Also removed tox dependency on mock and made the Flask dependency less strict. We now depend on any Flask release greater than 1.0.x. Signed-off-by: Aidan Delaney <adelaney21@bloomberg.net>
2 participants
Hi,
First of all, thank you for this module. It has saved us a good chunk of development time. I've updated this module so that it is Python 3 only. I've also added another test and updated it for Flask > 1.0. Hooks can now return their http status, which the runner is free to ignore, or return to the end-user. Please let me know if you'd like me to change this PR in any way.
Thank you.