event type use enum class #9
Conversation
higumachan
force-pushed
the
feature/use_enum
branch
from
0457a9d to
4d1108b
Compare
|
Thanks for the contribution @higumachan. My main thought when reviewing this is "Is this backwards compatible for existing users?" Besides that, please review our CONTRIBUTING.md; it requires signing of commits to acknowledge the DCO et al. Please could you squash your commits into a single commit too? |
|
Thanks for reply. This is not compatible for exiting users. I reviewed "CONTRIBUTING.md". |
I use python enum class on "event_type". (https://docs.python.org/3.5/library/enum.html) good points Easy to browse the event list If you use Pycharm or jedi or etc complementation tool then it will complement There feature is downward compatibility. Signed-Off-By: Yuta Hinokuma <[email protected]>
higumachan
force-pushed
the
feature/use_enum
branch
from
46bb044 to
ebb43f3
Compare
|
I fixed them. Please could you review again? |
There was a problem hiding this comment.
I wonder whether this change actually makes things more confusing for users, given there's now another indirection between what they see on the GitHub docs and here?
It seems there are two things that could be improved compared to what's on master:
- Making it easier for people to discover what hook names exist and what they do
- Making it harder for people to typo those names
For (1), if people aren't using an IDE that recognises this enum, then I think this PR actually makes things worse, since now they can't just look at the GitHub docs, they have to look at the source for this package. (Yes this PR backward compat supports the string format, but users probably won't know that)
For (2), instead of using enums, the package could instead just validate the names passed against a fixed list that matches the GitHub docs names. This would obviously have to be kept up to date, but no more so than this enum. An added bonus of this approach would be that it's not a backward incompatible change either.
| @@ -8,7 +8,7 @@ | |||
| author_email="[email protected], [email protected], [email protected], [email protected]", | |||
| license='Apache 2.0', | |||
| packages=["github_webhook"], | |||
| install_requires=['flask', 'six'], | |||
| install_requires=['flask', 'six', 'enum34'], | |||
There was a problem hiding this comment.
Perhaps use the python_version specifier here to limit this dependency to Python 2 only?
|
|
||
| class EventType(Enum): | ||
| """ | ||
| Event enum type |
There was a problem hiding this comment.
I think it would be good to link to https://developer.github.com/v3/activity/events/types/ in the docstring so it shows up in the docs, plus anyone keeping this event list up to date in the future knows where to look
| Repository = 'repository' | ||
| Status = 'status' | ||
| TeamAdd = 'team_add' | ||
| Watch = 'watch' |
There was a problem hiding this comment.
There are a half dozen events that are missing here compared to the list on:
https://developer.github.com/v3/activity/events/types/
I use python enum class on "event_type". (https://docs.python.org/3.5/library/enum.html)
good points