gh-543 Update regex to include hyphens in username #544
Conversation
|
Haven't done much python so I'm not sure I understand the build failures. Let me know if I can do anything else to help get this merged. |
| @@ -996,7 +996,8 @@ def pubsubhubbub(self, mode, topic, callback, secret=''): | |||
| :returns: bool | |||
| """ | |||
| from re import match | |||
| m = match('https?://[\w\d\-\.\:]+/\w+/[\w\._-]+/events/\w+', topic) | |||
| m = match('https?://[\w\d\-\.\:]+/\w+[\w-]+\w/[\w\._-]+/events/\w+', | |||
There was a problem hiding this comment.
Based on what you said GitHub's sign-up allows, shouldn't the username regexp be \w[\w-]+\w because then you'd be able to have a-b or a-bcdefg-h etc.?
There was a problem hiding this comment.
Upon further reflection I don't think we should validate too strictly what GitHub says a valid username is today. They might change it, right?
How about [\w-]+?
There was a problem hiding this comment.
It's unlikely that they'll change this. Fixing it later is also always easy and I'd like to not be too relaxed in the data we accept.
|
The test failures aren't your fault. We'll fix those separately once you've updated the PR with my comments. Thanks! |
|
Closing & re-opening should trigger a new merged build from Travis which should pass for you @jmathai |
7e3f9ea to
1b70765
Compare
|
@sigmavirus24 updated the regex per your comments, reverted the test I changed and added a few new ones. |
Fix bug where a username which contains a hyphen cannot be used in the `GitHub.pubsubhubbub` method.
|
Rebased. Should be ready for review/merge. |
|
Thanks @jmathai! 🍰 |
Fix bug where a username which contains a hyphen cannot be used in the
GitHub.pubsubhubbubmethod.