-
-
-
rbwebhooks/rbwebhooks/extension.py (Diff revision 1) I like how the connection / dispatching of signals is separated into its own class here.
-
rbwebhooks/rbwebhooks/extension.py (Diff revision 1) I brought up my concern about this being a blocking function in IRC. I think we're going to just accept that for now. Later down the line, we might have a proxy function that we can fork out so that RB doesn't block while sending this request...but I think that can wait for now.
-
rbwebhooks/rbwebhooks/extension.py (Diff revision 1) Hm - you're not just returning the default, you're also *writing* the default setting so that it's no longer a default. Is this intentional? If so, why?
-
rbwebhooks/rbwebhooks/models.py (Diff revision 1) Maybe this should be connected to the SignalHandlers class? So that we can define new signals in a single place?
-
rbwebhooks/rbwebhooks/models.py (Diff revision 1) If I'm not mistaken, verify_exists defaults to False, and is going to be deprecated in Django 1.5 (https://docs.djangoproject.com/en/dev/ref/models/fields/#django.db.models.URLField) So I think you can just have: url = models.URLField()
WebHooks Extension
Review Request #2855 — Created Feb. 10, 2012 and submitted
The start of a WebHooks extension. Currently configuration for URL targets is in the Admin Site for the extension. To modify the targets, click the Database button on the extension list. Only one hook has been implemented so far.
Tested on local dev by targeting a local instance of ReviewBot
Description | From | Last Updated |
---|---|---|
Just being nitpicky here, but shouldn't it be more like (empty lines are relevant): imports of urllib and urllib2 django … |
AM ammok | |
What is this global used for? |
mike_conley | |
Hm - you're not just returning the default, you're also *writing* the default setting so that it's no longer a … |
mike_conley | |
Maybe this should be connected to the SignalHandlers class? So that we can define new signals in a single place? |
mike_conley | |
If I'm not mistaken, verify_exists defaults to False, and is going to be deprecated in Django 1.5 (https://docs.djangoproject.com/en/dev/ref/models/fields/#django.db.models.URLField) So I … |
mike_conley | |
We'll need to make sure this gets taken out. Also, it might be a good idea to try hooking into … |
mike_conley | |
Do we go over 80 chars if we put these all on line 31? It just looks a bit odd … |
mike_conley | |
Why the extra newlines on line 11 / 14? |
mike_conley | |
This comment can probably be more clear. "Get the enabled RBWebHooksExtension instance (since we can assume if we've reached this … |
mike_conley | |
nit: I think this needs to be indented one more space, so that the curly braces line up. |
mike_conley | |
This comment can probably be fleshed out a bit, explaining what this extension does. |
mike_conley | |
This comment is probably superfluous. |
mike_conley | |
nit: Maybe just put that last parenthesis on the same line as the last argument? I guess I don't really … |
mike_conley | |
If we failed, we should probably log that we failed. |
mike_conley | |
Space here. |
ME medanat | |
This could be: while attempts: try: return urlib2.urlopen(request) except ...: attempts -= 1 |
ME medanat | |
A strict return False may not be needed, as a failed urlopen will return None. |
ME medanat | |
is_new = changedesc is None if not is_new: ... |
ME medanat | |
Space here. |
ME medanat |
-
This review request is missing your __init__.py, which, I presume, is because it is blank. (this is more of a heads-up for those who might wish to try this locally)
-
rbwebhooks/rbwebhooks/extension.py (Diff revision 1) Just being nitpicky here, but shouldn't it be more like (empty lines are relevant): imports of urllib and urllib2 django imports rbwebhooks imports reviewboard imports
-
rbwebhooks/rbwebhooks/extension.py (Diff revision 1) Another nitpick, but would this make more sense as the singular "_get_setting()"?
-
-
rbwebhooks/rbwebhooks/forms.py (Diff revision 1) I realize this might be more in the vein of giving people enough rope to hang themselves, but would 1 make a better min value?
-
rbwebhooks/rbwebhooks/views.py (Diff revision 1) Instead of using a global extension variable, I'm wondering if it would make sense to go through the ExtensionManager? i.e., from reviewboard.extensions.base import get_extension_manager extension_manager = get_extension_manager() extension = extension_manager.get_enabled_extension(RBWebHooksExtension.id)
Change Summary:
Changes from reviews have been made. Fixed configuration for extension.settings (retry attempts configuration). Moved signal handlers to separate file.
Diff: |
Revision 2 (+198) |
---|
Change Summary:
Updated description to reflect current state.
Description: |
|
---|
-
This continues to look good.
-
rbwebhooks/rbwebhooks/extension.py (Diff revision 2) We'll need to make sure this gets taken out. Also, it might be a good idea to try hooking into Review Board's logging mechanism for this sort of thing.
-
rbwebhooks/rbwebhooks/handlers.py (Diff revision 2) Do we go over 80 chars if we put these all on line 31? It just looks a bit odd broken up like this.
Change Summary:
Updated to send a JSON payload, including the information needed for ReviewBot. Extension now takes advantage of default_settings and its own Admin Site.
Description: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||||||||
Diff: |
Revision 3 (+239) |
-
Hey Steven, So I downloaded the patch for this and started fiddling with it, and I'm not seeing the admin site for WebHooks showing up in the admin interface. What am I doing wrong? -Mike
-
Hey Steven, Just a few minor comments, but overall this looks pretty good. -Mike
-
-
rbwebhooks/rbwebhooks/admin.py (Diff revision 3) This comment can probably be more clear. "Get the enabled RBWebHooksExtension instance (since we can assume if we've reached this code branch that the extension is enabled), and register WebHookTargetAdmin with the admin site." Something like that, perhaps.
-
rbwebhooks/rbwebhooks/admin_urls.py (Diff revision 3) nit: I think this needs to be indented one more space, so that the curly braces line up.
-
rbwebhooks/rbwebhooks/extension.py (Diff revision 3) This comment can probably be fleshed out a bit, explaining what this extension does.
-
-
rbwebhooks/rbwebhooks/extension.py (Diff revision 3) nit: Maybe just put that last parenthesis on the same line as the last argument? I guess I don't really like that last parenthesis hanging out in space. :p
-
-
rbwebhooks/rbwebhooks/extension.py (Diff revision 3) If we failed, we should probably log that we failed.
-
Change Summary:
Updated based on Mike's review, and cleaned things up a bit.
Diff: |
Revision 4 (+212) |
---|
Change Summary:
Removed the WIP summary
Summary: |
|
|||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
-
Small coding style issues, but otherwise this looks good! Well done. Yazan Medanat
-
-
rbwebhooks/rbwebhooks/extension.py (Diff revision 4) This could be: while attempts: try: return urlib2.urlopen(request) except ...: attempts -= 1
-
rbwebhooks/rbwebhooks/extension.py (Diff revision 4) A strict return False may not be needed, as a failed urlopen will return None.
-