Description: |
|
||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
SCM Web Hooks for reviewboard : Phase 1
Review Request #5510 — Created Feb. 19, 2014 and submitted
Information | |
---|---|
b.ramnani | |
Review Board | |
master | |
5596 | |
Reviewers | |
reviewboard, students | |
chipx86, david, mike_conley, smacleod |
Generate url infrastructure for hosting services so that they can be called to access web hooks. Added repo_urlpatterns to the service base class. Modified registration and unregistration of hosting services to add and remove urlpatterns respectively.
Wrote test case HostingServiceUrlPatternTests in hostingsvcs.tests.py. I've tested the addition and deletion of url patterns during registration and unregistration, respectively. Added tests for cases when the url_patterns are not defined. Also tested adding bitbucket url patterns (see screenshot). All tests have passed. :)
Description | From | Last Updated |
---|---|---|
Always do absolute imports. 'urls' could be anything. You also need to have this below with the project imports. |
|
|
Imports must always be alphabetical. |
|
|
Isn't this already in alphabetical order ? django... djblets..... |
B. b.ramnani | |
Name this repository_url_patterns. |
|
|
The summary has to be one line without wrapping. |
|
|
"thr" -> "the" |
|
|
Trailing whitespace. |
|
|
"URL" instead of "url" |
|
|
This can just be if cls.repo_urlpatterns. |
|
|
The parameters to patterns need to align. Spaces around the + |
|
|
Trailing whitespace. |
|
|
Alphabetical order. |
|
|
This must be one import statement, using parens, and one per line to prevent going over the line limit. |
|
|
Alphabetical. |
|
|
Two blank lines. |
|
|
This doesn't meet our formatting rules, and you're missing url(). It should be: repository_url_patterns = patterns( '', url('^hooks/pre-commit/$', ...), ) … |
|
|
Two blank lines. |
|
|
This is kind of hard to read. Can you group it into logical sections, with comments about what you're testing … |
|
|
You don't need to create your own client. You can just use self.client. |
|
|
URLs should always have a trailing /. Same below. |
|
|
Two blank lines. |
|
|
Two blank lines. |
|
|
You should just include dynamic_urls, and not do a new patterns(...) |
|
|
If I do that I get the following error: The included urlconf <DynamicURLResolver [] (None:None) > doesn't have any patterns … |
B. b.ramnani | |
This looks like a merge failure. |
|
|
Remove the trailing whitespace. In fact, revert this file. |
|
|
Merge failure. Revert this file. |
|
|
Revert this file too. |
|
|
Imports must always be alphabetical. |
|
|
No blank line between these. |
|
|
This needs to be a patterns(...) |
|
|
This should just be dummyView (which needs to be renamed to something more specific, and not use camelCase). |
|
|
You're missing the trailing slashes on these URLs, which will prevent you from getting to the right view. |
|
|
Review Board imports should go after the third-party module (Django, Djblets, etc.) imports, separated by a blank line. |
|
|
This should be in alphabetical order - include should be before patterns, and the line needs to be moved to … |
|
|
URL instead of url. |
|
|
These should be in alphabetical order, before from django.utils.import six. |
|
|
URL instead of url. |
|
|
URL instead of url. |
|
|
This should be formatted like this instead: repository_url_patterns = patterns( '', url(r'^hooks/pre-commit/$', hosting_service_url_test_view) ) |
|
|
Comments should start with a # and a single space, like: # This is a comment. Also, URL instead of … |
|
|
include should be before patterns. |
|
|
"URL pattern" "class" |
|
|
"KeyError" |
|
|
The '' should go on the next line. Spaces around the + |
|
|
I'd suggest moving this code after the try block. It's best to keep the bodies of a try/except as simple … |
|
|
Docstring summaries cannot wrap unless they are a unit test function. I'd actually recommend moving this into DummyService. You should … |
|
|
Should be 2 blank lines. |
|
|
This should be formatted like: repository_url_patterns = patterns( '', url(r'.....', view_name), ) |
|
|
"URL" Same any time it appears in a comment or string. |
|
|
Make sure you have this as the first line (with a blank line after): from __future__ import unicode_literals |
|
|
Blank line between these. |
|
|
Space after '' in patterns() |
|
|
This import isn't used. |
|
|
"URL" |
|
|
The last parenthesis should be on the next line, aligned with cls_urlpatterns on line 363. |
|
|
These lines are indented an extra level. |
|
|
"registering" |
|
|
This import isn't used. |
|
|
There should be a space after the comma, not before (so it should be ...patterns('', dynamic_urls)) |
|
|
There is extra whitespace before the parenthesis. |
|
|
Actually, this should just be removed entirely from the BitBucket hosting service, no? |
|
|
These lines are indented four too-many spaces. |
|
Change Summary:
Finished implementation of addition and deletion of urlpatterns in hosting services. added a test case. need some help in passing the test cases.
Description: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||
Branch: |
|
||||||||||||
Diff: |
Revision 2 (+1315 -1238) |
-
-
reviewboard/hostingsvcs/service.py (Diff revision 2) Always do absolute imports. 'urls' could be anything.
You also need to have this below with the project imports.
-
-
-
reviewboard/hostingsvcs/service.py (Diff revision 2) The summary has to be one line without wrapping.
-
-
-
-
reviewboard/hostingsvcs/service.py (Diff revision 2) The parameters to
patterns
need to align.Spaces around the
+
-
-
-
reviewboard/hostingsvcs/tests.py (Diff revision 2) This must be one import statement, using parens, and one per line to prevent going over the line limit.
-
-
-
reviewboard/hostingsvcs/tests.py (Diff revision 2) This doesn't meet our formatting rules, and you're missing
url()
. It should be:repository_url_patterns = patterns( '', url('^hooks/pre-commit/$', ...), )
Also note the
/$
that you need at the end. -
-
reviewboard/hostingsvcs/tests.py (Diff revision 2) This is kind of hard to read. Can you group it into logical sections, with comments about what you're testing in each case?
-
reviewboard/hostingsvcs/tests.py (Diff revision 2) URLs should always have a trailing
/
.Same below.
-
-
-
reviewboard/hostingsvcs/urls.py (Diff revision 2) You should just include
dynamic_urls
, and not do a newpatterns(...)
-
-
reviewboard/static/rb/css/reviews.less (Diff revision 2) Remove the trailing whitespace.
In fact, revert this file.
-
reviewboard/static/rb/js/views/commentDialogView.js (Diff revision 2) Merge failure. Revert this file.
-
-
-
-
I don't really have much to add to Christian's review, but I'll do another review on your next update!
-
-
-
reviewboard/hostingsvcs/urls.py (Diff revision 2) If I do that I get the following error:
The included urlconf <DynamicURLResolver [] (None:None) > doesn't have any patterns in it
Testing Done: |
|
---|
-
-
reviewboard/hostingsvcs/tests.py (Diff revision 2) You don't need to create your own client. You can just use
self.client
.
Change Summary:
Added dummyView to the tests file.
Branch: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+1322 -1238) |
-
-
-
reviewboard/hostingsvcs/tests.py (Diff revision 3) This should just be
dummyView
(which needs to be renamed to something more specific, and not use camelCase). -
reviewboard/hostingsvcs/tests.py (Diff revision 3) You're missing the trailing slashes on these URLs, which will prevent you from getting to the right view.
-
-
reviewboard/hostingsvcs/service.py (Diff revision 2) Isn't this already in alphabetical order ?
django...
djblets.....
Change Summary:
Fixed the test case for registration and unregistration of url patterns
Description: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||
Diff: |
Revision 4 (+1351 -1237) |
-
Here are just some minor style issues (a lot of which are duplicates)!
-
reviewboard/hostingsvcs/service.py (Diff revision 4) Review Board imports should go after the third-party module (Django, Djblets, etc.) imports, separated by a blank line.
-
reviewboard/hostingsvcs/service.py (Diff revision 4) This should be in alphabetical order -
include
should be beforepatterns
, and the line needs to be moved to being beforefrom django.utils import six
. -
-
reviewboard/hostingsvcs/tests.py (Diff revision 4) These should be in alphabetical order, before
from django.utils.import six
. -
-
-
reviewboard/hostingsvcs/tests.py (Diff revision 4) This should be formatted like this instead:
repository_url_patterns = patterns( '', url(r'^hooks/pre-commit/$', hosting_service_url_test_view) )
-
reviewboard/hostingsvcs/tests.py (Diff revision 4) Comments should start with a # and a single space, like:
# This is a comment.
Also, URL instead of url.
-
-
Home stretch! A few things to fix up now that the bulk of this is done.
-
-
-
reviewboard/hostingsvcs/service.py (Diff revision 5) The
''
should go on the next line.Spaces around the
+
-
reviewboard/hostingsvcs/service.py (Diff revision 5) I'd suggest moving this code after the try block. It's best to keep the bodies of a try/except as simple as possible.
-
reviewboard/hostingsvcs/tests.py (Diff revision 5) Docstring summaries cannot wrap unless they are a unit test function.
I'd actually recommend moving this into
DummyService
. You should be abe to make this a@classmethod
. -
-
reviewboard/hostingsvcs/tests.py (Diff revision 5) This should be formatted like:
repository_url_patterns = patterns( '', url(r'.....', view_name), )
-
reviewboard/hostingsvcs/tests.py (Diff revision 5) "URL"
Same any time it appears in a comment or string.
-
reviewboard/hostingsvcs/urls.py (Diff revision 5) Make sure you have this as the first line (with a blank line after):
from __future__ import unicode_literals
-
-
-
I've tested this out with my GitHub hook, and it works! :) While doing this, I noticed a few more really minor things.
-
-
-
reviewboard/hostingsvcs/service.py (Diff revision 6) The last parenthesis should be on the next line, aligned with
cls_urlpatterns
on line 363. -
-
-
-
reviewboard/hostingsvcs/urls.py (Diff revision 6) There should be a space after the comma, not before (so it should be
...patterns('', dynamic_urls)
) -
Change Summary:
Made the changes as reviewed by heapify. Added Bitbucket test.
Testing Done: |
|
||||||
---|---|---|---|---|---|---|---|
Diff: |
Revision 7 (+131 -3) |
||||||
Added Files: |
-
There's a typo in your change description: "called ro access web hooks"
-
reviewboard/hostingsvcs/bitbucket.py (Diff revision 7) These lines are indented four too-many spaces.
-
-
reviewboard/hostingsvcs/bitbucket.py (Diff revision 7) Actually, this should just be removed entirely from the BitBucket hosting service, no?
Summary: |
|
||||||
---|---|---|---|---|---|---|---|
Description: |
|