Summary

Providing URI templates for extensions

Review Request #8432 — Created Sept. 24, 2016 and submitted March 12, 2018, 12:44 p.m.

Information

Owner

maram

Repository

Djblets

Branch

master

Bugs

Depends On

Commit

3ef58d3...

Reviewers

Groups

djblets, students

People

chipx86

Description

This project adds uri templates for extensions to the api. Presently,

the RootResource web API resource generates URI templates as

shortcuts to users. Extensions are able to provide their own API

resources, however they are not currently picked up by RootResource.

It has been confirmed that this isn't a caching bug. Instead,

RootResource needs to be modified to continue walking the tree upon

encountering an extension. Currently, the proposed solution is to

create a mixin class incorporating functionality from RootResource

to track signals from the ExtensionResource. The Review Board project

RootResource would inherit this new mixin accordingly.

Testing Done

Performed an HTTP request on localhost:8080/api/ when no extensions are
installed and noted the absence of any extension templates in the response.

Installed the checklist extension from rb-extension-pack and enabled
it on my local instance. Made a new HTTP request to localhost:8080/api/
and noted that the extension templates are now correctly included.
Disabling the extension removes the extensions.

Ran djblets unit tests and noted all tests pass.

Adding new unit tests to verify functionality. All pass.

Issues

Description	From	Last Updated
test_extensionresourcemixin.py should be test_extension_resource_mixin.py.	brennie	Oct. 13, 2016, 2:36 p.m.
"Presently The" in your description should be "Presently the"	brennie	Oct. 25, 2016, 5:51 p.m.
I noticed your testing done mentions the httpie tool and the fact that the devserver is on port 8080. Can …	brennie	Oct. 25, 2016, 5:51 p.m.
"URI" in your summary.	brennie	Oct. 28, 2016, 1:51 p.m.
For the Description, "ReviewBoard" -> "Review Board".	mike_conley	Nov. 4, 2016, 8:37 p.m.
test_root_resource.py is missing a module docstring.	chipx86	Nov. 8, 2016, 10:36 a.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Sept. 24, 2016, 1 p.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	Sept. 24, 2016, 1 p.m.
Col: 1 W391 blank line at end of file	reviewbot	Sept. 24, 2016, 1 p.m.
Col: 69 E261 at least two spaces before inline comment	reviewbot	Sept. 24, 2016, 1:01 p.m.
Col: 80 E501 line too long (88 > 79 characters)	reviewbot	Sept. 24, 2016, 1:01 p.m.
Col: 59 W292 no newline at end of file	reviewbot	Sept. 24, 2016, 1:01 p.m.
Col: 19 W292 no newline at end of file	reviewbot	Sept. 24, 2016, 1:54 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Sept. 24, 2016, 1:55 p.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	Sept. 24, 2016, 1:55 p.m.
This will save you a lot of review changes. Blank line around blocks (like if, for, etc.).	LA larmiej	Sept. 29, 2016, 8:38 p.m.
Module imports go above symbol imports, so this should be like: import six from djblets.foo import bar...	brennie	Sept. 29, 2016, 3:25 p.m.
Missing a period.	brennie	Sept. 29, 2016, 3:25 p.m.
This should be the first line of the method, followed by a blank line.	brennie	Sept. 29, 2016, 8:38 p.m.
Col: 80 E501 line too long (83 > 79 characters)	reviewbot	Sept. 29, 2016, 8:38 p.m.
The comment should be a docstring.	brennie	Sept. 29, 2016, 3:30 p.m.
Missing docstring.	brennie	Sept. 29, 2016, 3:31 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Sept. 29, 2016, 8:39 p.m.
Mixins should inherit from object, otherwise it's just a subclass. The idea is that a mixin is a very small, …	chipx86	Sept. 29, 2016, 8:40 p.m.
I know this is a WIP change, but for here and other docstrings, make sure to follow the requirements in …	chipx86	Sept. 29, 2016, 9:51 p.m.
This should be at the top of the file.	chipx86	Sept. 29, 2016, 8:57 p.m.
With my comments further down, you won't need to clear this. Same in _remove_extension_urls_from_template.	chipx86	Sept. 29, 2016, 9:51 p.m.
This should just be a keyword argument in the method definition. Same further down.	chipx86	Sept. 29, 2016, 9:50 p.m.
Blank line between these (between code and blocks like if/for).	chipx86	Sept. 29, 2016, 8:58 p.m.
We use Django's version of six: from django.utils import six Note also that six (and Django's version) is a third-party …	chipx86	Sept. 29, 2016, 9:34 p.m.
Should be 2 blank lines.	chipx86	Sept. 29, 2016, 8:59 p.m.
Yep, this will need to be extracted, but there's another thing that's important to note as well. As currently written, …	chipx86	Oct. 4, 2016, 1:07 p.m.
Col: 49 E127 continuation line over-indented for visual indent	reviewbot	Oct. 6, 2016, 8:32 p.m.
__init__ for a subclass/mixin MUST call the superclass __init__ method, in this case: super(ExtensionRootResourceMixin, self).__init__()	brennie	Oct. 4, 2016, 8:58 p.m.
Col: 80 E501 line too long (85 > 79 characters)	reviewbot	Oct. 6, 2016, 8:32 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Oct. 6, 2016, 8:32 p.m.
Col: 33 E126 continuation line over-indented for hanging indent	reviewbot	Oct. 6, 2016, 8:32 p.m.
Col: 71 W292 no newline at end of file	reviewbot	Oct. 6, 2016, 8:32 p.m.
Col: 49 E127 continuation line over-indented for visual indent	reviewbot	Oct. 8, 2016, 4:56 p.m.
Col: 80 E501 line too long (85 > 79 characters)	reviewbot	Oct. 8, 2016, 4:56 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Oct. 8, 2016, 4:56 p.m.
Col: 33 E126 continuation line over-indented for hanging indent	reviewbot	Oct. 8, 2016, 4:56 p.m.
Col: 71 W292 no newline at end of file	reviewbot	Oct. 8, 2016, 4:56 p.m.
Col: 49 E127 continuation line over-indented for visual indent	reviewbot	Oct. 8, 2016, 4:56 p.m.
Col: 80 E501 line too long (85 > 79 characters)	reviewbot	Oct. 8, 2016, 4:56 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Oct. 8, 2016, 4:56 p.m.
'ipdb' imported but unused	reviewbot	Oct. 8, 2016, 4:56 p.m.
Col: 80 E501 line too long (88 > 79 characters)	reviewbot	Oct. 8, 2016, 4:56 p.m.
Col: 56 E261 at least two spaces before inline comment	reviewbot	Oct. 8, 2016, 4:56 p.m.
Col: 33 E126 continuation line over-indented for hanging indent	reviewbot	Oct. 8, 2016, 4:56 p.m.
Col: 71 W292 no newline at end of file	reviewbot	Oct. 8, 2016, 4:56 p.m.
Col: 49 E127 continuation line over-indented for visual indent	reviewbot	Oct. 8, 2016, 4:56 p.m.
Col: 80 E501 line too long (85 > 79 characters)	reviewbot	Oct. 8, 2016, 4:56 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Oct. 8, 2016, 4:56 p.m.
Col: 80 E501 line too long (83 > 79 characters)	reviewbot	Oct. 8, 2016, 4:56 p.m.
Col: 51 E261 at least two spaces before inline comment	reviewbot	Oct. 8, 2016, 4:56 p.m.
Col: 33 E126 continuation line over-indented for hanging indent	reviewbot	Oct. 8, 2016, 4:55 p.m.
Col: 12 E713 test for membership should be 'not in'	reviewbot	Oct. 8, 2016, 4:55 p.m.
Col: 12 E713 test for membership should be 'not in'	reviewbot	Oct. 8, 2016, 4:55 p.m.
Col: 49 E127 continuation line over-indented for visual indent	reviewbot	Oct. 8, 2016, 4:55 p.m.
Col: 80 E501 line too long (85 > 79 characters)	reviewbot	Oct. 8, 2016, 4:55 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Oct. 8, 2016, 4:55 p.m.
Col: 80 E501 line too long (83 > 79 characters)	reviewbot	Oct. 8, 2016, 4:54 p.m.
Col: 51 E261 at least two spaces before inline comment	reviewbot	Oct. 8, 2016, 4:54 p.m.
Col: 33 E126 continuation line over-indented for hanging indent	reviewbot	Oct. 8, 2016, 4:53 p.m.
Col: 5 E303 too many blank lines (2)	reviewbot	Oct. 8, 2016, 4:52 p.m.
You can do :py:class:djblets.resources.whatever.RootResource and it will link to the root resource documentation	brennie	Oct. 13, 2016, 2:39 p.m.
Missing docstring.	brennie	Oct. 13, 2016, 2:50 p.m.
I don't think this tells us anything.	brennie	Oct. 13, 2016, 2:54 p.m.
No blank line here.	brennie	Oct. 13, 2016, 2:54 p.m.
This is pretty unweildly. I think it would be better to pull this out into a variable and iterate over …	brennie	Oct. 13, 2016, 2:59 p.m.
This should be '%s%s' % (href, registered_href). Generally, you'll want to avoid string concatenation in Python because it is slow. …	brennie	Oct. 13, 2016, 3:04 p.m.
"Yield" Docstrings for methods should always be written in the imperative mood, i.e., they tell (or command) you what to …	brennie	Oct. 13, 2016, 3:06 p.m.
Missing Args and Yields Yields is just like Returns.	brennie	Oct. 13, 2016, 3:20 p.m.
Again, you'll want to use %-formatting here, as this will create a temporary string (object_href + child.uri_name) and then create …	brennie	Oct. 13, 2016, 3:21 p.m.
Missing Args.	brennie	Oct. 13, 2016, 3:48 p.m.
Here, too.	brennie	Oct. 13, 2016, 3:49 p.m.
The first import should be: from __future__ import unicode_literals This turns all 'string's into u'string's automatically. The str (default for …	brennie	Oct. 13, 2016, 3:52 p.m.
'ExtensionResource' imported but unused	reviewbot	Oct. 15, 2016, 7:44 p.m.
Docstring.	brennie	Oct. 13, 2016, 3:55 p.m.
Missing tests.	brennie	Oct. 22, 2016, 10:37 p.m.
Docstring. This should be something like """Unit tests for root resource template registration."""	brennie	Oct. 13, 2016, 3:57 p.m.
Please don't use mock.Mock. Create a dummy class that has all the stuff you need. mock is a huge dependency …	brennie	Oct. 13, 2016, 4:15 p.m.
Col: 13 E122 continuation line missing indentation or outdented	reviewbot	Oct. 15, 2016, 7:44 p.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	Oct. 15, 2016, 7:44 p.m.
Col: 80 E501 line too long (91 > 79 characters)	reviewbot	Oct. 15, 2016, 7:44 p.m.
Col: 80 E501 line too long (85 > 79 characters)	reviewbot	Oct. 15, 2016, 7:44 p.m.
Col: 80 E501 line too long (91 > 79 characters)	reviewbot	Oct. 15, 2016, 7:44 p.m.
"URI template cache"	brennie	Oct. 13, 2016, 3:57 p.m.
Col: 80 E501 line too long (92 > 79 characters)	reviewbot	Oct. 15, 2016, 7:44 p.m.
Col: 80 E501 line too long (92 > 79 characters)	reviewbot	Oct. 15, 2016, 7:44 p.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	Oct. 15, 2016, 7:44 p.m.
Col: 80 E501 line too long (92 > 79 characters)	reviewbot	Oct. 15, 2016, 7:44 p.m.
Col: 80 E501 line too long (92 > 79 characters)	reviewbot	Oct. 15, 2016, 7:44 p.m.
Col: 68 W292 no newline at end of file	reviewbot	Oct. 15, 2016, 7:44 p.m.
Additionally, docstrings should be of the form: """Single line summary. Multi-line description. """ You may want to cut out the …	brennie	Oct. 15, 2016, 6:44 p.m.
This should line up with the first ".	brennie	Oct. 15, 2016, 6:44 p.m.
Same here.	brennie	Oct. 15, 2016, 6:47 p.m.
Docstrings should be in the imperitive moode ("Generate"). Missing Args, Returns blocks	brennie	Oct. 15, 2016, 6:54 p.m.
And here, too.	brennie	Oct. 15, 2016, 6:54 p.m.
And here. Probably want to re-word this to be something like """Return the associated extension resource."""	brennie	Oct. 15, 2016, 6:55 p.m.
Since this is soo long, you'll probably want to pull out the six.iteritems bit into resource_templates.	brennie	Oct. 15, 2016, 7:03 p.m.
Backslashes are icky and can lead to bugs when there is trailing whitespace that accidentally committed, so we tend to …	brennie	Oct. 15, 2016, 7:06 p.m.
unicode.	brennie	Oct. 15, 2016, 7:06 p.m.
This actually yields a tuple. However, I feel you'll want to use a namedtuple here. Somewhere above in this file …	brennie	Oct. 15, 2016, 7:40 p.m.
Can we pull self.walk_resources(child, child_href) into a variable and iterate over that instead?	brennie	Oct. 15, 2016, 7:40 p.m.
Pull this out, too.	brennie	Oct. 15, 2016, 7:40 p.m.
Imperitive mood ("Register")	brennie	Oct. 15, 2016, 7:07 p.m.
unicode.	brennie	Oct. 15, 2016, 7:10 p.m.
unicode	brennie	Oct. 15, 2016, 7:10 p.m.
"Unregister" This may not be a word, but what do dictionaries know? They let frobnicate be a word :)	brennie	Oct. 15, 2016, 7:10 p.m.
unicode	brennie	Oct. 15, 2016, 7:10 p.m.
Not really necessary tbh.	brennie	Oct. 15, 2016, 7:11 p.m.
"""Unit tests for the ExtensionRootResourceMixin.""" No need for full class paths here.	brennie	Oct. 15, 2016, 7:11 p.m.
setUp must call super(ExtensionTemplateTests, self).setUp() or you're going to end up with weird, unsolvable test failures sometimes :)	brennie	Oct. 15, 2016, 7:12 p.m.
Docstrings should be of the form: """Single line summary. Multi-line description. """ See my comment on the ExtensionTemplateTests for what …	brennie	Oct. 15, 2016, 7:14 p.m.
setUp must call super(RootResourceTemplateRegistrationTests, self).setUp() or you're going to end up with weird, unsolvable test failures sometimes :)	brennie	Oct. 15, 2016, 7:14 p.m.
No blank line here.	brennie	Oct. 15, 2016, 7:17 p.m.
Pull this out to a top level element, otherwise we are creating a new class each test pass. If you …	brennie	Oct. 15, 2016, 7:17 p.m.
Col: 13 E122 continuation line missing indentation or outdented	reviewbot	Oct. 15, 2016, 7:43 p.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	Oct. 15, 2016, 7:42 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Oct. 15, 2016, 7:42 p.m.
Col: 80 E501 line too long (91 > 79 characters)	reviewbot	Oct. 15, 2016, 7:42 p.m.
Col: 80 E501 line too long (85 > 79 characters)	reviewbot	Oct. 15, 2016, 7:42 p.m.
Col: 80 E501 line too long (91 > 79 characters)	reviewbot	Oct. 15, 2016, 7:43 p.m.
Col: 80 E501 line too long (92 > 79 characters)	reviewbot	Oct. 15, 2016, 7:43 p.m.
Col: 80 E501 line too long (92 > 79 characters)	reviewbot	Oct. 15, 2016, 7:43 p.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	Oct. 15, 2016, 7:43 p.m.
Col: 80 E501 line too long (92 > 79 characters)	reviewbot	Oct. 15, 2016, 7:43 p.m.
Col: 80 E501 line too long (92 > 79 characters)	reviewbot	Oct. 15, 2016, 7:43 p.m.
Col: 55 E201 whitespace after '('	reviewbot	Oct. 25, 2016, 5:52 p.m.
Col: 51 E202 whitespace before ')'	reviewbot	Oct. 25, 2016, 5:52 p.m.
'SpyAgency' imported but unused	reviewbot	Oct. 25, 2016, 5:55 p.m.
Col: 80 E501 line too long (87 > 79 characters)	reviewbot	Oct. 25, 2016, 5:53 p.m.
Col: 30 E126 continuation line over-indented for hanging indent	reviewbot	Oct. 25, 2016, 5:54 p.m.
Blank line between these.	brennie	Oct. 25, 2016, 5:56 p.m.
Missing Args	brennie	Oct. 26, 2016, 10:09 a.m.
Missing **kwargs (dict) Also, what is kwargs used for here?	brennie	Oct. 25, 2016, 6 p.m.
Since this isnt a type object, can we rename this to ext or extension?	brennie	Oct. 25, 2016, 6 p.m.
Since walk_resources returns the namedtuple instance and we're not using all the fields we can do: for entry in self.walk_resources(resource, …	brennie	Oct. 25, 2016, 9:30 p.m.
Can we pull out self.get_extension_resource() into a variable at the top of the function and reuse it in each iteration?	brennie	Oct. 25, 2016, 9:31 p.m.
Same here re: kwargs.	brennie	Oct. 25, 2016, 6:01 p.m.
Same here.	brennie	Oct. 25, 2016, 6:01 p.m.
This will fit on one line.	brennie	Oct. 26, 2016, 2:27 p.m.
Reformat as: self.register_uri_template(entry.name, entry.list_href, ext_resource)	brennie	Oct. 26, 2016, 2:27 p.m.
Same thing here.	brennie	Oct. 25, 2016, 9:32 p.m.
Here, too.	brennie	Oct. 25, 2016, 9:32 p.m.
This will fit on one line	brennie	Oct. 26, 2016, 2:27 p.m.
This will fit on one line.	brennie	Oct. 26, 2016, 2:27 p.m.
Python standard libraries go in between __future__ imports and 3rd party imports (like django and djblets).	brennie	Oct. 25, 2016, 6:02 p.m.
"Attributes".	brennie	Oct. 25, 2016, 6:02 p.m.
Can you reformat this as: #: Attributes #: field (type): #: Description of field... #: #: field (type): #: Description …	brennie	Oct. 25, 2016, 6:04 p.m.
This should actually be the full type, i.e. djblets.webapi.responses.WebAPIResponseError.	brennie	Oct. 25, 2016, 6:06 p.m.
djblets.webapi.resources.base.WebAPIResource	brennie	Oct. 26, 2016, 2:28 p.m.
"Whether or not the resource is a list resource."	brennie	Oct. 26, 2016, 2:29 p.m.
Col: 55 E201 whitespace after '('	reviewbot	Oct. 25, 2016, 5:55 p.m.
Col: 51 E202 whitespace before ')'	reviewbot	Oct. 25, 2016, 5:55 p.m.
Blank line between these.	brennie	Oct. 25, 2016, 6:06 p.m.
Could you reformat this as: templates[registered_name] = ( '%s%s' % (href, registered_href) )	brennie	Oct. 26, 2016, 2:30 p.m.
This should yield self.ResourceEntry(resource.name_plural, list_href, resource, True).	brennie	Oct. 25, 2016, 9:32 p.m.
This should yield self.ResourceEntry(name, list_href, resource, is_list).	brennie	Oct. 25, 2016, 6:08 p.m.
And here	brennie	Oct. 25, 2016, 6:08 p.m.
And here.	brennie	Oct. 25, 2016, 6:08 p.m.
Reformat as: yield self.ResourceEntry(resource.name, object_href, resource, False)	brennie	Oct. 26, 2016, 2:30 p.m.
This can be simplified to: for entry in self.walk_resources(child, child_href): yield entry	brennie	Oct. 26, 2016, 2:31 p.m.
Can we rename this relative_path	brennie	Oct. 25, 2016, 6:10 p.m.
Can we name this relative_resource or parent_resource?	brennie	Oct. 25, 2016, 6:10 p.m.
Blank line between these.	brennie	Oct. 25, 2016, 6:11 p.m.
Again, can we rename this relative_resource or parent_resource?	brennie	Oct. 25, 2016, 6:11 p.m.
Reformat as: self._registered_uri_templates[relative_resource][name] = \ relative_path	brennie	Oct. 26, 2016, 2:39 p.m.
Docstrings for all these	brennie	Oct. 25, 2016, 6:17 p.m.
Can we call this TestChildResourceOne	brennie	Oct. 25, 2016, 6:18 p.m.
TestChildResourceTwo	brennie	Oct. 25, 2016, 6:19 p.m.
TestResourceOne	brennie	Oct. 25, 2016, 6:19 p.m.
TestResourceTwo	brennie	Oct. 25, 2016, 9:32 p.m.
TestExtension	brennie	Oct. 25, 2016, 6:20 p.m.
TestRootResource	brennie	Oct. 25, 2016, 6:20 p.m.
Docstrings should be of the form """Single line summary. Multi-line description. """	brennie	Oct. 25, 2016, 6:24 p.m.
Rename to expected_result, actual_result. Also, I think this would be better formatted as the following: expected_result = { 'extension': 'http://testserver/{extension_name}/', …	brennie	Oct. 25, 2016, 8:55 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Oct. 25, 2016, 5:55 p.m.
Add a trailing comma.	brennie	Oct. 25, 2016, 6:27 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Oct. 25, 2016, 5:55 p.m.
Docstrings should be of the form """Single line summary. Multi-line description. """	brennie	Oct. 25, 2016, 6:27 p.m.
You should be providing the extension resource to this, e.g. self.ext_mgr = ExtensionManager('') self.ext_resource = ExtensionResource(self.ext_mgr) self.root_resource = RootResource([ext_resource])	brennie	Oct. 25, 2016, 8:58 p.m.
Can we just call these .ext_mgr and .extension?	brennie	Oct. 25, 2016, 9:03 p.m.
Reformat this as: self.root_resource._registered_uri_templates = { None: {'extension-1': '/first/'}, self.mock_extension: { 'extensions': 'http://localhost:8080/api/extensions/', }, } Also, isn't this backwards? Shouldn't …	brennie	Oct. 25, 2016, 9:05 p.m.
Rename as actual_result, expected_result ?	brennie	Oct. 25, 2016, 9:07 p.m.
Reformat this as: self.root_resource._uri_templates = { 'key1': 'value', 'key2': 'value', }	brennie	Oct. 25, 2016, 9:33 p.m.
Rename as expected_result, actual_result?	brennie	Oct. 25, 2016, 9:10 p.m.
This should be formatted as: self.assertFalse(self.root_resource._registered_uri_tempaltes[ self.mock_extension])	brennie	Oct. 25, 2016, 9:11 p.m.
Col: 30 E126 continuation line over-indented for hanging indent	reviewbot	Oct. 25, 2016, 5:55 p.m.
Reformat this as: self.root_resource._uri_templates = { 'key1': 'value', 'key2': 'value', }	brennie	Oct. 25, 2016, 9:12 p.m.
Reformat this as: self.root_resource._uri_templates = { 'key1': 'value', 'key2': 'value', }	brennie	Oct. 25, 2016, 9:12 p.m.
Can you make a note that the subclass that uses this mixin must implement this?	brennie	Oct. 26, 2016, 10:17 p.m.
How about: for name, href in six.iteritems(unassigned_templates): # ....	brennie	Oct. 26, 2016, 10:17 p.m.
for entry in self.walk_resources(self, base_href):	brennie	Oct. 26, 2016, 11:10 p.m.
How about: if is_list: list_templates = self._registered_uri_templates.get( resource, {}) for name, href in six.iteritems(list_templates): templates[name] = ( '%s%s' % (entry.href, …	brennie	Oct. 26, 2016, 11:12 p.m.
This can be simplified to for entry in self.walk_resources(child, child_href): yield entry	brennie	Oct. 27, 2016, 12:46 p.m.
for entry in self.walk_resources(child, child_href): yield entry	brennie	Oct. 26, 2016, 11:15 p.m.
This doesn't really describe this properly. I would say it's a mixin for root resources that make use of ExtensionResource. …	chipx86	Oct. 27, 2016, 9:02 p.m.
Blank line between these.	chipx86	Oct. 27, 2016, 5:12 p.m.
This is missing **kwargs.	chipx86	Oct. 27, 2016, 5:13 p.m.
The type needs to be a full class path.	chipx86	Oct. 27, 2016, 5:13 p.m.
Same comments as above.	chipx86	Oct. 27, 2016, 5:14 p.m.
This is missing a "Returns" block that states the return type and content.	chipx86	Oct. 27, 2016, 5:15 p.m.
This should be on the same line.	chipx86	Oct. 27, 2016, 5:15 p.m.
Same line.	chipx86	Oct. 27, 2016, 5:16 p.m.
This can be one line.	chipx86	Oct. 27, 2016, 5:16 p.m.
Indentation level for "Args" and "Yields" is wrong.	chipx86	Oct. 27, 2016, 5:17 p.m.
ResourceEntry needs to: 1) Be on the next line 2) Be RootResource.ResourceEntry (these need to be either absolute paths, if …	chipx86	Oct. 27, 2016, 9:04 p.m.
Let's construct all these with keyword arguments instead of positional arguments for the values.	chipx86	Oct. 27, 2016, 9:08 p.m.
This should remain one statement.	chipx86	Oct. 27, 2016, 9:09 p.m.
Same comment as above for keyword arguments.	chipx86	Oct. 27, 2016, 9:09 p.m.
"URI"	chipx86	Oct. 27, 2016, 5:23 p.m.
_registered_uri_templates and _uri_templates are implementation details. Docs should focus on a higher level saying what this does, not how it …	chipx86	Oct. 27, 2016, 9:15 p.m.
This should say what it's relative to.	chipx86	Oct. 27, 2016, 10:28 p.m.
Needs to be an absolute class path. Should also include , optional after the type.	chipx86	Oct. 27, 2016, 5:31 p.m.
You can simplify much of this and reduce dictionary accesses: templates = self._registered_uri_templates.setdefault(relative_resource, {}) templates[name] = relative_path	chipx86	Oct. 27, 2016, 9:35 p.m.
These are pretty conflicting statements. The second overrides the first.	chipx86	Oct. 27, 2016, 9:32 p.m.
"URI"	chipx86	Oct. 27, 2016, 5:32 p.m.
Same as above with the doc content. It should be higher level than what internal fields are being modified.	chipx86	Oct. 27, 2016, 9:20 p.m.
Full class path and , optional.	chipx86	Oct. 27, 2016, 5:32 p.m.
"URI"	chipx86	Oct. 27, 2016, 5:32 p.m.
Better to do: try: del self._registered_uri_templates[relative_resource][name] except KeyError: pass This does two things: 1) Doesn't fail (like the current code) …	chipx86	Oct. 27, 2016, 5:34 p.m.
Blank line between these. Djblets is the current project, and so has its own import group.	chipx86	Oct. 27, 2016, 5:35 p.m.
These can be one import statement.	chipx86	Oct. 27, 2016, 5:35 p.m.
Please use numbers instead of spelling them out for these classes.	chipx86	Oct. 27, 2016, 7:09 p.m.
Missing a trailing period. Same with the ones below.	chipx86	Oct. 27, 2016, 7:10 p.m.
This isn't in the proper form. It should be: """ ... Returns djblets.extensions.resources.ExtensionResource <description here> """ That said, these are …	chipx86	Oct. 27, 2016, 7:13 p.m.
This should probably be ExtensionRootResourceMixinTests.	chipx86	Oct. 27, 2016, 7:13 p.m.
This should be done first, probably. Blank line between it and the setters.	chipx86	Oct. 27, 2016, 7:15 p.m.
So two things: 1) When testing a method, the docstring needs to list the class name and the proper name …	chipx86	Oct. 27, 2016, 8:20 p.m.
This whole thing should be used inline below, like: self.assertEqual( actual_result, { ... })	chipx86	Oct. 27, 2016, 8:36 p.m.
Should just use assertEqual, not assert*Equal methods. Also, flip these. Thing you're testing should be first. Expected value second. Same …	chipx86	Oct. 27, 2016, 8:36 p.m.
This should also be inline below. Same with other tests.	chipx86	Oct. 27, 2016, 8:36 p.m.
This should be RootResourceTests, and should live in its own file (test_root_resource.py).	chipx86	Oct. 27, 2016, 8:41 p.m.
Consistency is good. Both of these should look the same: .... = { self.ext_resource: { 'extensions': ..., }, None: { …	chipx86	Oct. 27, 2016, 8:42 p.m.
This can be one line. Should ideally use keyword arguments, which would push to two lines, but they should start …	chipx86	Oct. 27, 2016, 9:24 p.m.
This can be one line.	chipx86	Oct. 27, 2016, 8:44 p.m.
These calls will read better if you use keyword arguments.	chipx86	Oct. 27, 2016, 9:23 p.m.
Should be inline.	chipx86	Oct. 27, 2016, 8:46 p.m.
Condense this.	chipx86	Oct. 27, 2016, 8:54 p.m.
Col: 50 E225 missing whitespace around operator	reviewbot	Oct. 28, 2016, 12:26 a.m.
Col: 9 E303 too many blank lines (2)	reviewbot	Oct. 28, 2016, 12:26 a.m.
This is not a class, it is an instance (isn't it?) Rename this to ext or extension. If it is …	brennie	Oct. 28, 2016, 2:31 p.m.
"the URI templates."	brennie	Oct. 28, 2016, 1:57 p.m.
Same here.	brennie	Oct. 28, 2016, 2:34 p.m.
"the URI templates."	brennie	Oct. 28, 2016, 1:59 p.m.
Blank line between these.	brennie	Oct. 28, 2016, 2 p.m.
Can you change this to be a @classmethod ? e.g. @classmethod def walk_resources(cls, resource, list_href): and replace all usages of …	brennie	Oct. 28, 2016, 2:01 p.m.
Please elaborate on this.	brennie	Oct. 29, 2016, 2:33 p.m.
Don't wrap this. You will get a error from review bot but you can drop it.	brennie	Oct. 28, 2016, 2:02 p.m.
Blank line between these	brennie	Oct. 28, 2016, 2:02 p.m.
I actually don't think we normally document args and *kwargs, since I often think that it's for passing through arguments …	mike_conley	Nov. 6, 2016, 1:09 p.m.
This is an interesting documentation style choice, the #:. Can you explain why you made that choice? I'm not familiar …	mike_conley	Nov. 6, 2016, 1:09 p.m.
Leftovers?	mike_conley	Nov. 4, 2016, 9:06 p.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	Oct. 29, 2016, 2:57 p.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	Oct. 29, 2016, 2:57 p.m.
This (and all other references to djblets.extensions.Extension) needs to be djblets.extensions.extension.Extension). The former doesn't exist, and only the latter will …	chipx86	Nov. 8, 2016, 10:37 a.m.
Public methods must come before private method. This should be right after __init__.	chipx86	Nov. 8, 2016, 10:37 a.m.
No parens.	chipx86	Nov. 8, 2016, 10:38 a.m.
No outer parens needed.	chipx86	Nov. 8, 2016, 10:41 a.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	Nov. 6, 2016, 1:30 p.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	Nov. 6, 2016, 1:30 p.m.
No parens.	chipx86	Nov. 8, 2016, 10:56 a.m.
This is indented one level too far.	chipx86	Nov. 20, 2016, 1:57 p.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	Nov. 8, 2016, 5:10 p.m.
Can you add a comment here just saying that we're clearing the cache so that future lookups will take into …	chipx86	Nov. 20, 2016, 1:59 p.m.
Missing trailing period. All comments should be in full sentence format.	chipx86	Feb. 2, 2018, 2:33 p.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	Nov. 8, 2016, 5:10 p.m.
Same comment here as above with a comment.	chipx86	Nov. 20, 2016, 1:59 p.m.
Same here.	chipx86	Feb. 2, 2018, 10:56 a.m.
This doesn't need to be a raw string.	brennie	Nov. 29, 2016, 9:54 a.m.
Again, this doesn't need to be a raw string.	brennie	Nov. 29, 2016, 9:54 a.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	Nov. 21, 2016, 9:30 a.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	Nov. 21, 2016, 9:30 a.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	Dec. 3, 2016, 12:20 p.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	Dec. 3, 2016, 12:20 p.m.
E501 line too long (89 > 79 characters)	reviewbot	Feb. 2, 2018, 11:05 a.m.
E501 line too long (89 > 79 characters)	reviewbot	Feb. 2, 2018, 11:05 a.m.

Tool: Pyflakes
Processed Files:
    djblets/extensions/resources.py
    djblets/webapi/resources/root.py
    djblets/webapi/resources/mixins/extensions.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/extensions/resources.py
    djblets/webapi/resources/root.py
    djblets/webapi/resources/mixins/extensions.py

djblets/webapi/resources/mixins/extensions.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
djblets/webapi/resources/mixins/extensions.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```
djblets/webapi/resources/mixins/extensions.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W391 blank line at end of file
```
djblets/webapi/resources/root.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 69
 E261 at least two spaces before inline comment
```
djblets/webapi/resources/root.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (88 > 79 characters)
```
djblets/webapi/resources/root.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 59
 W292 no newline at end of file
```

Commit:

-	0366f7e0bcc68737bea9d72892e3713ed3335ae3
+	31fa8a2f73830519d5c8338f7d33f206699191b1

Diff:

Revision 2 (+31)

Show changes

	djblets/webapi/resources/root.py
	djblets/webapi/resources/mixins/extensions.py

Tool: Pyflakes
Processed Files:
    djblets/webapi/resources/root.py
    djblets/webapi/resources/mixins/extensions.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/webapi/resources/root.py
    djblets/webapi/resources/mixins/extensions.py

djblets/webapi/resources/mixins/extensions.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 19
 W292 no newline at end of file
```
djblets/webapi/resources/root.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```
djblets/webapi/resources/root.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```

Summary:

-	[WIP] Super rough - Providing uri templates for extensions
+	[WIP] Providing uri templates for extensions

djblets/webapi/resources/root.py (Diff revision 2)

The issue has been resolved. Show all issues

This will save you a lot of review changes.

Blank line around blocks (like if, for, etc.).

Change Summary:

Fully functional prototyping done. Next step will be to extract this code in RootResource to the Mixin class discussed during the code sprint. Will also need to investigate a means for testing.

Commit:

-	31fa8a2f73830519d5c8338f7d33f206699191b1
+	61963b35f9cec79897b252cb918381697f3cb7b1

Diff:

Revision 3 (+35)

Show changes

	djblets/webapi/resources/root.py
	djblets/webapi/resources/mixins/extensions.py

Tool: PEP8 Style Checker
Processed Files:
    djblets/webapi/resources/root.py
    djblets/webapi/resources/mixins/extensions.py



Tool: Pyflakes
Processed Files:
    djblets/webapi/resources/root.py
    djblets/webapi/resources/mixins/extensions.py

djblets/webapi/resources/root.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (83 > 79 characters)
```

djblets/webapi/resources/root.py (Diff revision 3)

The issue has been resolved. Show all issues

Module imports go above symbol imports, so this should be like:


import six
from djblets.foo import bar...

djblets/webapi/resources/root.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Missing a period.
```

djblets/webapi/resources/root.py (Diff revision 3)

The issue has been resolved. Show all issues

This should be the first line of the method, followed by a blank line.

rswanson Sept. 29, 2016, 3:31 p.m.

Since Christian had suggested extracting all of the extension code to its own Mixin class, I was planning to just include an import at the top of the script.

chipx86 Sept. 29, 2016, 3:32 p.m.

Remember that this class isn't going to be using the mixin. Review Board's RootResource will be. The mixin would be in djblets/extensions/resources.py.

djblets/webapi/resources/root.py (Diff revision 3)
The issue has been resolved. Show all issues
```
The comment should be a docstring.
```
djblets/webapi/resources/root.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Missing docstring.
```

Commit:

-	61963b35f9cec79897b252cb918381697f3cb7b1
+	c57b85bc4802812563eb5f04c135e2b6b00ce1f0

Diff:

Revision 4 (+49 -4)

Show changes

	djblets/extensions/resources.py
	djblets/webapi/resources/root.py

Tool: Pyflakes
Processed Files:
    djblets/extensions/resources.py
    djblets/webapi/resources/root.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/extensions/resources.py
    djblets/webapi/resources/root.py

djblets/extensions/resources.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```

This looks like a good start! There are some things that will need to change though.

I cover how mixins and inheritence work below, along with some tips on docstrings and some other Pythonisms to be aware of (less important for a WIP, but worth getting in the habit of sooner rather than later).

There are some bigger topics involving how URIs are computed and how the classes relate, and we'll need to go over that. I cover it further below.

djblets/extensions/resources.py (Diff revision 4)

The issue has been resolved. Show all issues

Mixins should inherit from object, otherwise it's just a subclass.

The idea is that a mixin is a very small, tight piece of code with (ideally) no parents, allowing other subclasses to include them without enforcing a particular parent class, which may not be desired.

As part of that contract, consumers of the mixin do have to inherit from a suitable parent (RootResource), but are free to choose which they want to use (some hypothetical SuperAwesomeRootResource would suffice, for instance).

rswanson Sept. 29, 2016, 9:51 p.m.

I see - coming from Java I was convinced that the mixin would need to inherit from RootResource in order to make use of the walk_resources method. That's really cool the language is as open ended as you've described.

djblets/extensions/resources.py (Diff revision 4)

The issue has been resolved. Show all issues

I know this is a WIP change, but for here and other docstrings, make sure to follow the requirements in this guide by the time it's out of WIP:

https://www.reviewboard.org/docs/codebase/dev/docs/writing-codebase-docs/

RS

rswanson Sept. 29, 2016, 9:51 p.m.
```
I'll be sure to follow it!
```

djblets/extensions/resources.py (Diff revision 4)
The issue has been resolved. Show all issues
```
This should be at the top of the file.
```

djblets/extensions/resources.py (Diff revision 4)

The issue has been resolved. Show all issues

With my comments further down, you won't need to clear this. Same in _remove_extension_urls_from_template.

djblets/extensions/resources.py (Diff revision 4)
The issue has been resolved. Show all issues
```
This should just be a keyword argument in the method definition. Same further down.
```
1. RS
  
  rswanson Sept. 29, 2016, 9:51 p.m.
  TIL about keyword arguments!
djblets/extensions/resources.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Blank line between these (between code and blocks like if/for).
```

djblets/webapi/resources/root.py (Diff revision 4)

The issue has been resolved. Show all issues

We use Django's version of six:

from django.utils import six


Note also that six (and Django's version) is a third-party module, not a Python stdlib module, so it shouldn't go in the stdlib import group.

djblets/webapi/resources/root.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Should be 2 blank lines.
```

djblets/webapi/resources/root.py (Diff revision 4)

The issue has been resolved. Show all issues

Yep, this will need to be extracted, but there's another thing that's important to note as well. As currently written, it appears that the URI templates will end using something like /api/myextension.Extension/myresource/ for the URI template, which wouldn't be correct. Rather, it should be /api/extensions/myextension.Extension/myresource/ (or /s/my-site/api/extensions/myextension.Extension/myresource/ for Local Sites). The parent (/api/extensions/) depends on how the consumer has things set up.

So there's a couple things that need to be solved here:

1) We need a clean way of registering URI templates from the mixin without clearing dictionaries e from therand without RootResource knowing it's going to be modified by a mixin in some way (_incomplete_uri_templates).

2) We need a way to inject these particular URI templates at the right level.

For #1...

I propose adding two new methods to RootResource: register_uri_template(name, rel_path) and unregister_uri_template(name).

These would operate off of _incomplete_uri_templates, allowing that to stay internal. (I would maybe rename that at this point to _registered_uri_templates). Registering/unregistering would update this dictionary, and clear self._uri_templates to ensure the old cache isn't used.

For #2...

We need to be able to generate the right path to the /api/extensions/ instance without knowing where that's going to be, and without RootResource knowing anything about it, but we can't easily figure out that path in the extension signal handlers because we don't have a request (which we need in order to get the right path). Tricky!

The best place to compute that path is in RootResource (since it's dealing with a request, without it explicitly knowing about ExtensionResource, and supporting maybe other scenarios like this in the future with other specialized resources.

What I propose there is to take our shiny new methods and add a new optional keyword argument: relative_to_resource=. This would default to None, but could take a resource instance (like resources.extension from Review Board). So like:

def register_uri_template(name, rel_path, relative_to_resource=None):
    ...

That dictionary of registered URI templates would be keyed off by the parent resource (None or an instance), rather than the extension ID like it is now. For example, printing it could look something like:

_registered_uri_templates = {
    None: {
        'my-resource': '/blahblah/some-resource',
        ...
    },
    resources.extension: {
        'my-extension-resource': '/myextension.Extension/myresource/',
        ...
    },
    ...
}

You'd then have walk_resources include 2 more things in its yield statements: resource, and a bool indicating if it's a list resource being yielded.

Then, get_uri_templates can do:

for registered_name, registered_href in six.iteritems(self._registered_uri_templates.get(None, [])):
    templates[registered_name] = registered_href

for name, href, resource, is_list in self.walk_resources(self, base_href):
    templates[name] = href

    if is_list:
        for registered_name, registered_href in six.iteritems(self._registered_uri_templates.get(resource, [])):
            templates[registered_name] = href + registered_href

That should cleanly allow callers to register new URI templates at either the root level or relative to any other resource, without knowing where they are in the tree. RootResource does the work of figuring that out and building the URLs correctly.

Mixin and ExtensionResource

There is one last part that needs to then be solved: The mixin needs to know about the ExtensionResource instance, but remember, Review Board creates that, not Djblets!

To solve that, the mixin's contract with its consumer could be that the consumer must override a method: get_extension_resource. This would return the resources.extension instance (in the case of Review Board), allowing the mixin to simply call that on self and pass the result as the value to relative_to_resource=.

The mixin would provide a default that raises NotImplementedError. You can find other examples where we do this.

I know that's a lot! But I think this gives us everything we need, and keeps the separation at the right levels.

rswanson Sept. 29, 2016, 9:51 p.m.

Wow! that's a lot of information! Thanks for the help, I'm going to give it one (or five) more read throughs tonight and try making the adjustments in earnest. A question that remains once this refactoring is complete is finding the best way to test this - however that can wait until this first phase is done.

rswanson Oct. 4, 2016, 11:07 a.m.

Sorry about the delay in addressing this issue. I'll address suggestions in line.

For suggestion #1)

Makes total sense, I've started adjusting my RR accordingly.

For suggestion #2)

Optional keyword arguments are a good idea. Your suggestion of using the parent resource as a dictionary key makes sense - but I'm a bit confused as to how I should modify the walk_resources method to get the required information.

I'll include my latest RR after I'm happy with the changes and point out areas where I'm still unsure with my changes.

Thanks again for your review!

Change Summary:

I've addressed Christian's review comments - however I've caught a bit of a snag in that making http requests to the development server doesn't seem to be updating my template list anymore. While I investigate I thought I'd take the chance to show my progress.

Commit:

-	c57b85bc4802812563eb5f04c135e2b6b00ce1f0
+	c6cbd871e9b73ca761e6d20da149ccabd1c17d97

Diff:

Revision 5 (+93 -8)

Show changes

	djblets/extensions/resources.py
	djblets/webapi/resources/root.py

Tool: PEP8 Style Checker
Processed Files:
    djblets/extensions/resources.py
    djblets/webapi/resources/root.py



Tool: Pyflakes
Processed Files:
    djblets/extensions/resources.py
    djblets/webapi/resources/root.py

djblets/extensions/resources.py (Diff revision 5)
The issue has been dropped. Show all issues
```
Col: 49
 E127 continuation line over-indented for visual indent
```
djblets/extensions/resources.py (Diff revision 5)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (85 > 79 characters)
```
djblets/extensions/resources.py (Diff revision 5)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```
djblets/webapi/resources/root.py (Diff revision 5)
The issue has been dropped. Show all issues
```
Col: 33
 E126 continuation line over-indented for hanging indent
```
djblets/webapi/resources/root.py (Diff revision 5)
The issue has been dropped. Show all issues
```
Col: 71
 W292 no newline at end of file
```

djblets/extensions/resources.py (Diff revision 5)

The issue has been resolved. Show all issues

__init__ for a subclass/mixin MUST call the superclass __init__ method, in this case:

super(ExtensionRootResourceMixin, self).__init__()

Change Summary:

Since refactoring to a mixin, the signal code for _generate_extension_uris_for_template/_remove_extension_uris_from_template is no longer executing when adding an extension. I've added print statements for debugging purposes.

Commit:

-	c6cbd871e9b73ca761e6d20da149ccabd1c17d97
+	34ce1a112aee97c850c9a379e328c78d3d6e50f2

Diff:

Revision 6 (+96 -8)

Show changes

	djblets/extensions/resources.py
	djblets/webapi/resources/root.py

Tool: Pyflakes
Processed Files:
    djblets/extensions/resources.py
    djblets/webapi/resources/root.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/extensions/resources.py
    djblets/webapi/resources/root.py

djblets/extensions/resources.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 49
 E127 continuation line over-indented for visual indent
```
djblets/extensions/resources.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (85 > 79 characters)
```
djblets/extensions/resources.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```
djblets/webapi/resources/root.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 33
 E126 continuation line over-indented for hanging indent
```
djblets/webapi/resources/root.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 71
 W292 no newline at end of file
```

Commit:

-	34ce1a112aee97c850c9a379e328c78d3d6e50f2
+	5023fbcf0559d5afb07c557ab5e3067aa20f1456

Diff:

Revision 7 (+109 -15)

Show changes

	djblets/extensions/resources.py
	djblets/webapi/resources/base.py
	djblets/webapi/resources/root.py

Tool: Pyflakes
Processed Files:
    djblets/webapi/resources/base.py
    djblets/extensions/resources.py
    djblets/webapi/resources/root.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/webapi/resources/base.py
    djblets/extensions/resources.py
    djblets/webapi/resources/root.py

djblets/extensions/resources.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Col: 49
 E127 continuation line over-indented for visual indent
```
djblets/extensions/resources.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (85 > 79 characters)
```
djblets/extensions/resources.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```
djblets/webapi/resources/base.py (Diff revision 7)
The issue has been resolved. Show all issues
```
 'ipdb' imported but unused
```
djblets/webapi/resources/base.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (88 > 79 characters)
```
djblets/webapi/resources/base.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Col: 56
 E261 at least two spaces before inline comment
```
djblets/webapi/resources/root.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Col: 33
 E126 continuation line over-indented for hanging indent
```
djblets/webapi/resources/root.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Col: 71
 W292 no newline at end of file
```

Change Summary:

registered_uri_templates is now populating correctly - the final step to getting this refactoring operational again will be to modify get_uri_templates to correctly add the URIs of extensions.

Commit:

-	5023fbcf0559d5afb07c557ab5e3067aa20f1456
+	faf0a640e3badefb431a1548412fd85ea9335572

Diff:

Revision 8 (+111 -15)

Show changes

	djblets/extensions/resources.py
	djblets/webapi/resources/base.py
	djblets/webapi/resources/root.py

Tool: Pyflakes
Processed Files:
    djblets/webapi/resources/base.py
    djblets/extensions/resources.py
    djblets/webapi/resources/root.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/webapi/resources/base.py
    djblets/extensions/resources.py
    djblets/webapi/resources/root.py

djblets/extensions/resources.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Col: 49
 E127 continuation line over-indented for visual indent
```
djblets/extensions/resources.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (85 > 79 characters)
```
djblets/extensions/resources.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```
djblets/webapi/resources/base.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (83 > 79 characters)
```
djblets/webapi/resources/base.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Col: 51
 E261 at least two spaces before inline comment
```
djblets/webapi/resources/root.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Col: 33
 E126 continuation line over-indented for hanging indent
```
djblets/webapi/resources/root.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Col: 12
 E713 test for membership should be 'not in'
```
djblets/webapi/resources/root.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Col: 12
 E713 test for membership should be 'not in'
```

Commit:

-	faf0a640e3badefb431a1548412fd85ea9335572
+	dd237b500341b8e055246b5f159fe5d5dd98c9d5

Diff:

Revision 9 (+117 -16)

Show changes

	djblets/extensions/resources.py
	djblets/webapi/resources/base.py
	djblets/webapi/resources/root.py

Tool: Pyflakes
Processed Files:
    djblets/webapi/resources/base.py
    djblets/extensions/resources.py
    djblets/webapi/resources/root.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/webapi/resources/base.py
    djblets/extensions/resources.py
    djblets/webapi/resources/root.py

djblets/extensions/resources.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Col: 49
 E127 continuation line over-indented for visual indent
```
djblets/extensions/resources.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (85 > 79 characters)
```
djblets/extensions/resources.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```
djblets/webapi/resources/base.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (83 > 79 characters)
```
djblets/webapi/resources/base.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Col: 51
 E261 at least two spaces before inline comment
```
djblets/webapi/resources/root.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Col: 33
 E126 continuation line over-indented for hanging indent
```
djblets/webapi/resources/root.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Col: 5
 E303 too many blank lines (2)
```

Commit:

-	dd237b500341b8e055246b5f159fe5d5dd98c9d5
+	23ada3bd341b671c5e683f8aaab5d78202e8f04f

Diff:

Revision 10 (+95 -9)

Show changes

	djblets/extensions/resources.py
	djblets/webapi/resources/root.py

Tool: Pyflakes
Processed Files:
    djblets/extensions/resources.py
    djblets/webapi/resources/root.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/extensions/resources.py
    djblets/webapi/resources/root.py

Change Summary:

Refactoring of RootResource to mixin complete. Currently investigating unit tests.

Summary:

-	[WIP] Providing uri templates for extensions
+	Providing uri templates for extensions

Testing Done:

	+	Performed `http :8080/api/` request when no extensions are installed and noted the absence of any extension templates in the response.
	+
	+	Installed the checklist extension from rb-extension-pack and enabled it on my local instance. Made a new `http :8080/api/` request and noted that the extension templates are now correctly included. Disabling the extension removes the extensions.

Change Summary:

Added tests to previous review.

Testing Done:

~		Performed `http :8080/api/` request when no extensions are installed and noted the absence of any extension templates in the response.
	~	Performed `http :8080/api/` request when no extensions are installed
	+	and noted the absence of any extension templates in the response.

~		Installed the checklist extension from rb-extension-pack and enabled it on my local instance. Made a new `http :8080/api/` request and noted that the extension templates are now correctly included. Disabling the extension removes the extensions.
	~	Installed the checklist extension from rb-extension-pack and enabled
	+	it on my local instance. Made a new `http :8080/api/` request and
	+	noted that the extension templates are now correctly included.
	+	Disabling the extension removes the extensions.
	+
	+	Ran djblets unit tests and noted all tests pass.
	+
	+	Adding new unit tests to verify functionality.

Commit:

-	23ada3bd341b671c5e683f8aaab5d78202e8f04f
+	855137c046fda860509fd765157a86b628f8729a

Diff:

Revision 11 (+167 -9)

Show changes

	djblets/extensions/resources.py
	djblets/webapi/resources/root.py
	djblets/webapi/tests/test_extensionresourcemixin.py

Tool: Pyflakes
Processed Files:
    djblets/extensions/resources.py
    djblets/webapi/tests/test_extensionresourcemixin.py
    djblets/webapi/resources/root.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/extensions/resources.py
    djblets/webapi/tests/test_extensionresourcemixin.py
    djblets/webapi/resources/root.py

djblets/webapi/tests/test_extensionresourcemixin.py (Diff revision 11)
The issue has been resolved. Show all issues
```
 'ExtensionResource' imported but unused
```
djblets/webapi/tests/test_extensionresourcemixin.py (Diff revision 11)
The issue has been resolved. Show all issues
```
Col: 13
 E122 continuation line missing indentation or outdented
```
djblets/webapi/tests/test_extensionresourcemixin.py (Diff revision 11)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (89 > 79 characters)
```
djblets/webapi/tests/test_extensionresourcemixin.py (Diff revision 11)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (91 > 79 characters)
```
djblets/webapi/tests/test_extensionresourcemixin.py (Diff revision 11)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (85 > 79 characters)
```
djblets/webapi/tests/test_extensionresourcemixin.py (Diff revision 11)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (91 > 79 characters)
```
djblets/webapi/tests/test_extensionresourcemixin.py (Diff revision 11)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (92 > 79 characters)
```
djblets/webapi/tests/test_extensionresourcemixin.py (Diff revision 11)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (92 > 79 characters)
```
djblets/webapi/tests/test_extensionresourcemixin.py (Diff revision 11)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (89 > 79 characters)
```
djblets/webapi/tests/test_extensionresourcemixin.py (Diff revision 11)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (92 > 79 characters)
```
djblets/webapi/tests/test_extensionresourcemixin.py (Diff revision 11)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (92 > 79 characters)
```
djblets/webapi/tests/test_extensionresourcemixin.py (Diff revision 11)
The issue has been resolved. Show all issues
```
Col: 68
 W292 no newline at end of file
```

The issue has been resolved. Show all issues

test_extensionresourcemixin.py should be test_extension_resource_mixin.py.

djblets/extensions/resources.py (Diff revision 11)

The issue has been resolved. Show all issues

You can do :py:class:djblets.resources.whatever.RootResource and it will link to the root resource documentation

brennie Oct. 11, 2016, 5:39 p.m.

Eep, stupid markdown.

This should be:

:py:class:`djblets.resources.whatever.RootResource`

djblets/extensions/resources.py (Diff revision 11)
The issue has been resolved. Show all issues
```
Missing docstring.
```
djblets/extensions/resources.py (Diff revision 11)
The issue has been resolved. Show all issues
```
I don't think this tells us anything.
```
1. RS
  
  rswanson Oct. 13, 2016, 4:15 p.m.
  Agreed.
djblets/webapi/resources/root.py (Diff revision 11)
The issue has been resolved. Show all issues
```
No blank line here.
```

djblets/webapi/resources/root.py (Diff revision 11)

This could also be:

template.extend({
    registered_name: '%s%s' % (href, registered_href),
    for registered_name, registered_href in six.iteritems(
        self._registered_uri_templates.get(resource, {})
    )
})


Though I'm not sure if I prefer that over the loop. Not an issue, just a consideration.

rswanson Oct. 13, 2016, 4:15 p.m.

Interesting! Thanks for offering it as a suggestion. I personally feel like the current loop code is easier to grasp (albeit ugly as you mentioned above).

djblets/webapi/resources/root.py (Diff revision 11)

The issue has been resolved. Show all issues

This is pretty unweildly. I think it would be better to pull this out into a variable and iterate over six.iteritems(foo) instead.

(Generally we discourage multi-line for expressions because they become rather unreadable.)

RS

rswanson Oct. 13, 2016, 4:15 p.m.
```
Makes sense.
```

djblets/webapi/resources/root.py (Diff revision 11)

The issue has been resolved. Show all issues

This should be '%s%s' % (href, registered_href).

Generally, you'll want to avoid string concatenation in Python because it is slow. Like, super slow. Using %-formatting will result in fewer allocations.

RS

rswanson Oct. 13, 2016, 4:15 p.m.
```
Mental Note.
```

djblets/webapi/resources/root.py (Diff revision 11)

A multiline dosctring would be

"""Long description.
"""

without the blank line. However, you'll need the blank line given you also need args/yields.

djblets/webapi/resources/root.py (Diff revision 11)

The issue has been resolved. Show all issues

"Yield"
Docstrings for methods should always be written in the imperative mood, i.e., they tell (or command) you what to do rather than describe what it is going to do. "Shut the door" (imperitive) vs "Shuts the door" (non-imperitive).

djblets/webapi/resources/root.py (Diff revision 11)
The issue has been resolved. Show all issues
```
Missing Args and Yields
Yields is just like Returns.
```

djblets/webapi/resources/root.py (Diff revision 11)

The issue has been resolved. Show all issues

Again, you'll want to use %-formatting here, as this will create a temporary string (object_href + child.uri_name) and then create a final string which is the result.

djblets/webapi/resources/root.py (Diff revision 11)
The issue has been resolved. Show all issues
```
Missing Args.
```
djblets/webapi/resources/root.py (Diff revision 11)
The issue has been resolved. Show all issues
```
Here, too.
```

djblets/webapi/tests/test_extensionresourcemixin.py (Diff revision 11)

The issue has been resolved. Show all issues

The first import should be:

from __future__ import unicode_literals


This turns all 'string's into u'string's automatically. The str (default for strings without unicode_literals) is actually a binary type and will cause encoding headaches.
Aside: if you ever want to use a binary string (str) literal, you can use b'foo').

This file will also need a docstring, e.g.

"""Unit tests for the classname mixin."""

RS

rswanson Oct. 13, 2016, 4:15 p.m.
```
That makes a lot of sense.
```

djblets/webapi/tests/test_extensionresourcemixin.py (Diff revision 11)
The issue has been resolved. Show all issues
```
Docstring.
```

djblets/webapi/tests/test_extensionresourcemixin.py (Diff revision 11)

The issue has been resolved. Show all issues

Missing tests.

rswanson Oct. 13, 2016, 4:15 p.m.

Yeah this is intentional - I'm Still trying to figure out what actually makes sense to test, and was hoping to get some guidance.

brennie Oct. 13, 2016, 4:25 p.m.

For testing you'll probably need

at least one extension providing a resource
a dummy root resource inheriting from the mixin

and then you can call the methods on that dummy root resource with the extension enabled/disabled and check the uri_templates

djblets/webapi/tests/test_extensionresourcemixin.py (Diff revision 11)

The issue has been resolved. Show all issues

Docstring. This should be something like

"""Unit tests for root resource template registration."""

djblets/webapi/tests/test_extensionresourcemixin.py (Diff revision 11)

The issue has been resolved. Show all issues

Please don't use mock.Mock. Create a dummy class that has all the stuff you need.
mock is a huge dependency and we're trying to get rid of it.

djblets/webapi/tests/test_extensionresourcemixin.py (Diff revision 11)
The issue has been resolved. Show all issues
```
"URI template cache"
```

Change Summary:

Addressing comments from @brennie.

Commit:

-	855137c046fda860509fd765157a86b628f8729a
+	fcad11f084d57e6bee87df25dc4bd55f85981f4c

Diff:

Revision 12 (+230 -10)

Show changes

	djblets/extensions/resources.py
	djblets/webapi/resources/root.py
	djblets/webapi/tests/test_extension_resource_mixin.py

Tool: Pyflakes
Processed Files:
    djblets/extensions/resources.py
    djblets/webapi/tests/test_extension_resource_mixin.py
    djblets/webapi/resources/root.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/extensions/resources.py
    djblets/webapi/tests/test_extension_resource_mixin.py
    djblets/webapi/resources/root.py

djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Col: 13
 E122 continuation line missing indentation or outdented
```
djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (89 > 79 characters)
```
djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (91 > 79 characters)
```
djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (85 > 79 characters)
```
djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (91 > 79 characters)
```
djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (92 > 79 characters)
```
djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (92 > 79 characters)
```
djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (89 > 79 characters)
```
djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (92 > 79 characters)
```
djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (92 > 79 characters)
```

Make sure to drop/mark as fixed old issues from review bot. they just clutter up the review request issue table if theyre out of date.

djblets/extensions/resources.py (Diff revision 12)

The issue has been resolved. Show all issues

Additionally, docstrings should be of the form:

"""Single line summary.

Multi-line description.
"""

You may want to cut out the :py:class: ref or move it into the description so you have an easier time fitting stuff in the summary.

You can also do:

"""...

.. seealso:
   :py:class:`~djblets.webapi.resources.root.RootResource`
"""

We use Sphinx for our documentation formatting. Here's a cheat sheet.

djblets/extensions/resources.py (Diff revision 12)
The issue has been resolved. Show all issues
```
This should line up with the first ".
```
djblets/extensions/resources.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Same here.
```

djblets/extensions/resources.py (Diff revision 12)

The issue has been resolved. Show all issues

Docstrings should be in the imperitive moode ("Generate").

Missing Args, Returns blocks

rswanson Oct. 15, 2016, 7:40 p.m.

This doesn't return anything - so is a Returns block necessary?

djblets/extensions/resources.py (Diff revision 12)
The issue has been resolved. Show all issues
```
And here, too.
```

djblets/extensions/resources.py (Diff revision 12)

The issue has been resolved. Show all issues

And here.

Probably want to re-word this to be something like
"""Return the associated extension resource."""

djblets/webapi/resources/root.py (Diff revision 12)

The issue has been resolved. Show all issues

Since this is soo long, you'll probably want to pull out the six.iteritems bit into resource_templates.

djblets/webapi/resources/root.py (Diff revision 12)

The issue has been resolved. Show all issues

Backslashes are icky and can lead to bugs when there is trailing whitespace that accidentally committed, so we tend to avoid them when it makes sense to do so. In this case, consider:

templates[registered_name] = (
    '%s%s'
    % (href, registered_href)
)

djblets/webapi/resources/root.py (Diff revision 12)
The issue has been resolved. Show all issues
```
unicode.
```

djblets/webapi/resources/root.py (Diff revision 12)

The issue has been resolved. Show all issues

This actually yields a tuple. However, I feel you'll want to use a namedtuple here.
Somewhere above in this file you'll want to define a tuple type like:

from collections import namedtuple


#: A resource entry returned from :py:meth:`RootResource.walk_resources`.
#:
#: Attribtues:
#:     name (unicode): ...
#:     list_href (unicode): ...
#:     resource (...): ...
#:     is_list (bool): ...
ResourceEntry = namedtuple('ResourceEntry', ('name', 'list_href', 'resource', 'is_list')


then you can Yields: ResourceEntry and document the ResourceEntry as above.

rswanson Oct. 15, 2016, 7:40 p.m.

I've changed my code to make use of namedtuples as you suggest - let me know if my intuition is correct with how they should be used.

djblets/webapi/resources/root.py (Diff revision 12)

The issue has been resolved. Show all issues

Can we pull self.walk_resources(child, child_href) into a variable and iterate over that instead?

djblets/webapi/resources/root.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Pull this out, too.
```
djblets/webapi/resources/root.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Imperitive mood ("Register")
```
djblets/webapi/resources/root.py (Diff revision 12)
The issue has been resolved. Show all issues
```
unicode.
```
djblets/webapi/resources/root.py (Diff revision 12)
The issue has been resolved. Show all issues
```
unicode
```
djblets/webapi/resources/root.py (Diff revision 12)
The issue has been resolved. Show all issues
```
"Unregister"
This may not be a word, but what do dictionaries know? They let frobnicate be a word :)
```
1. RS
  
  rswanson Oct. 15, 2016, 7:40 p.m.
  +1
djblets/webapi/resources/root.py (Diff revision 12)
The issue has been resolved. Show all issues
```
unicode
```
djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Not really necessary tbh.
```
djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 12)
The issue has been resolved. Show all issues
```
"""Unit tests for the ExtensionRootResourceMixin."""
No need for full class paths here.
```

djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 12)

The issue has been resolved. Show all issues

setUp must call super(ExtensionTemplateTests, self).setUp() or you're going to end up with weird, unsolvable test failures sometimes :)

djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 12)

The issue has been resolved. Show all issues

Docstrings should be of the form:

"""Single line summary.

Multi-line description.
"""

See my comment on the ExtensionTemplateTests for what is required here (i.e., not this much :))

djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 12)

The issue has been resolved. Show all issues

setUp must call super(RootResourceTemplateRegistrationTests, self).setUp() or you're going to end up with weird, unsolvable test failures sometimes :)

djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 12)
The issue has been resolved. Show all issues
```
No blank line here.
```

djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 12)

The issue has been resolved. Show all issues

Pull this out to a top level element, otherwise we are creating a new class each test pass. If you want, you can do:

class RootResourceTemplateRegistrationTests(TestCase):
    """...."""

    class MockExtensionManager(object):
        pass

    def setUp(self):
        self.root_resource = RootResource()
        self.extension_manager = self.MockExtensionmanager()
        # ...


Additionally, this should inherit from object to be a new style class, which are more sensible.

Commit:

-	fcad11f084d57e6bee87df25dc4bd55f85981f4c
+	a6337b8718588f5f52e1ce38f4d144ac4156da4a

Diff:

Revision 13 (+286 -11)

Show changes

	djblets/extensions/resources.py
	djblets/webapi/resources/root.py
	djblets/webapi/tests/test_extension_resource_mixin.py

Change Summary:

Added final tests to test_extension_resource_mixin.py and fixed an error I'd introduced in a previous patchset.

Testing Done:

		Performed `http :8080/api/` request when no extensions are installed
		and noted the absence of any extension templates in the response.

		Installed the checklist extension from rb-extension-pack and enabled
		it on my local instance. Made a new `http :8080/api/` request and
		noted that the extension templates are now correctly included.
		Disabling the extension removes the extensions.

		Ran djblets unit tests and noted all tests pass.

~		Adding new unit tests to verify functionality.
	~	Adding new unit tests to verify functionality. All pass.

Commit:

-	a6337b8718588f5f52e1ce38f4d144ac4156da4a
+	41551cece689f1b80529c221259796c7512392a3

Diff:

Revision 14 (+309 -11)

Show changes

	djblets/extensions/resources.py
	djblets/webapi/resources/root.py
	djblets/webapi/tests/test_extension_resource_mixin.py

djblets/extensions/resources.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Blank line between these.
```
djblets/extensions/resources.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Missing Args
```
1. RS
  
  rswanson Oct. 25, 2016, 9:33 p.m.
  I'm not actually sure how to document args/kwargs in this case.

djblets/extensions/resources.py (Diff revision 14)

The issue has been resolved. Show all issues

Missing **kwargs (dict)
Also, what is kwargs used for here?

rswanson Oct. 25, 2016, 6:28 p.m.

Nothing. I didn't think about it until now - but I'd misunderstood the use of keyword arguments. Kwargs doesn't actually need to be here.

djblets/extensions/resources.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Since this isnt a type object, can we rename this to ext or extension?
```

djblets/extensions/resources.py (Diff revision 14)

The issue has been resolved. Show all issues

Since walk_resources returns the namedtuple instance and we're not using all the fields we can do:

        for entry in self.walk_resources(resource, partial_href):
            self.register_uri_template(entry.name, entry.href, ext_resource)

djblets/extensions/resources.py (Diff revision 14)

The issue has been resolved. Show all issues

Can we pull out self.get_extension_resource() into a variable at the top of the function and reuse it in each iteration?

djblets/extensions/resources.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Same here re: kwargs.
```
1. RS
  
  rswanson Oct. 25, 2016, 6:28 p.m.
  Same reason above. I've removed it.
djblets/extensions/resources.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Same here.
```
djblets/extensions/resources.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Same thing here.
```
djblets/extensions/resources.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Here, too.
```

djblets/webapi/resources/root.py (Diff revision 14)

The issue has been resolved. Show all issues

Python standard libraries go in between __future__ imports and 3rd party imports (like django and djblets).

djblets/webapi/resources/root.py (Diff revision 14)
The issue has been resolved. Show all issues
```
"Attributes".
```

djblets/webapi/resources/root.py (Diff revision 14)

The issue has been resolved. Show all issues

Can you reformat this as:

#: Attributes
#:     field (type):
#:         Description of field...
#:
#:     field (type):
#:         Description of field...

It makes it a bit easier to scan

djblets/webapi/resources/root.py (Diff revision 14)

The issue has been resolved. Show all issues

This should actually be the full type, i.e.
djblets.webapi.responses.WebAPIResponseError.

djblets/webapi/resources/root.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Blank line between these.
```

djblets/webapi/resources/root.py (Diff revision 14)

The issue has been resolved. Show all issues

This should yield self.ResourceEntry(resource.name_plural, list_href, resource, True).

djblets/webapi/resources/root.py (Diff revision 14)
The issue has been resolved. Show all issues
```
This should yield self.ResourceEntry(name, list_href, resource, is_list).
```
djblets/webapi/resources/root.py (Diff revision 14)
The issue has been resolved. Show all issues
```
And here
```
djblets/webapi/resources/root.py (Diff revision 14)
The issue has been resolved. Show all issues
```
And here.
```
djblets/webapi/resources/root.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Can we rename this relative_path
```
djblets/webapi/resources/root.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Can we name this relative_resource or parent_resource?
```
djblets/webapi/resources/root.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Blank line between these.
```
djblets/webapi/resources/root.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Again, can we rename this relative_resource or parent_resource?
```

djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 14)

The issue has been resolved. Show all issues

Docstrings for all these

djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Can we call this TestChildResourceOne
```
djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 14)
The issue has been resolved. Show all issues
```
TestChildResourceTwo
```
djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 14)
The issue has been resolved. Show all issues
```
TestResourceOne
```
djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 14)
The issue has been resolved. Show all issues
```
TestResourceTwo
```
djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 14)
The issue has been resolved. Show all issues
```
TestExtension
```
djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 14)
The issue has been resolved. Show all issues
```
TestRootResource
```

djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 14)

The issue has been resolved. Show all issues

Docstrings should be of the form

"""Single line summary.

Multi-line description.
"""

djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 14)

The issue has been resolved. Show all issues

Rename to expected_result, actual_result.

Also, I think this would be better formatted as the following:

        expected_result = {
            'extension': 'http://testserver/{extension_name}/',
            'extensions': 'http://testserver/extensions/',
            'mock_resource_1s': (
                'http://testserver/extensions/djblets.webapi.tests.'
                'test_extension_resource_mixin.MockExtension/mock-resource-1s/'
            ),
            'mock_resource_2s': (
                'http://testserver/extensions/djblets.webapi.tests.
                'test_extension_resource_mixin.MockExtension/mock-resource-2s/'
            ),
        }

djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Add a trailing comma.
```

djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 14)

The issue has been resolved. Show all issues

Docstrings should be of the form

"""Single line summary.

Multi-line description.
"""

djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 14)

The issue has been resolved. Show all issues

You should be providing the extension resource to this, e.g.

self.ext_mgr = ExtensionManager('')
self.ext_resource = ExtensionResource(self.ext_mgr)
self.root_resource = RootResource([ext_resource])

djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Can we just call these .ext_mgr and .extension?
```

djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 14)

The issue has been resolved. Show all issues

Reformat this as:

self.root_resource._registered_uri_templates = {
    None: {'extension-1': '/first/'},
    self.mock_extension: {
        'extensions': 'http://localhost:8080/api/extensions/',
    },
}


Also, isn't this backwards? Shouldn't this be:
{
    self.mock_extension: {'extension-1': '/first/'},
    None: {'extensions': 'http://localhost:8080/api/extensions'},
}

rswanson Oct. 25, 2016, 9:33 p.m.

The order shouldn't matter here - but if you'd rather I put None second I can do that.

djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Rename as actual_result, expected_result ?
```

djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 14)

The issue has been resolved. Show all issues

Reformat this as:

self.root_resource._uri_templates = {
    'key1': 'value',
    'key2': 'value',
}

djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Rename as expected_result, actual_result?
```

djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 14)

The issue has been resolved. Show all issues

This should be formatted as:


self.assertFalse(self.root_resource._registered_uri_tempaltes[
    self.mock_extension])

djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 14)

The issue has been resolved. Show all issues

Reformat this as:

self.root_resource._uri_templates = {
    'key1': 'value',
    'key2': 'value',
}

djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 14)

The issue has been resolved. Show all issues

Reformat this as:

self.root_resource._uri_templates = {
    'key1': 'value',
    'key2': 'value',
}

The issue has been resolved. Show all issues

"Presently The" in your description should be "Presently the"

The issue has been resolved. Show all issues

I noticed your testing done mentions the httpie tool and the fact that the devserver is on port 8080. Can you reword it so its more like

"Made an HTTP GET request to /foo/bar/whatsits/ and saw foobars"

Tool: PEP8 Style Checker
Processed Files:
    djblets/extensions/resources.py
    djblets/webapi/tests/test_extension_resource_mixin.py
    djblets/webapi/resources/root.py



Tool: Pyflakes
Processed Files:
    djblets/extensions/resources.py
    djblets/webapi/tests/test_extension_resource_mixin.py
    djblets/webapi/resources/root.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/extensions/resources.py
    djblets/webapi/tests/test_extension_resource_mixin.py
    djblets/webapi/resources/root.py

djblets/webapi/resources/root.py (Diff revision 13)
The issue has been resolved. Show all issues
```
Col: 55
 E201 whitespace after '('
```
djblets/webapi/resources/root.py (Diff revision 13)
The issue has been resolved. Show all issues
```
Col: 51
 E202 whitespace before ')'
```
djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 13)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (87 > 79 characters)
```
djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 13)
The issue has been resolved. Show all issues
```
Col: 30
 E126 continuation line over-indented for hanging indent
```
djblets/webapi/resources/root.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Col: 55
 E201 whitespace after '('
```
djblets/webapi/resources/root.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Col: 51
 E202 whitespace before ')'
```
djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```
djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```
djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Col: 30
 E126 continuation line over-indented for hanging indent
```

Tool: Pyflakes
Processed Files:
    djblets/extensions/resources.py
    djblets/webapi/tests/test_extension_resource_mixin.py
    djblets/webapi/resources/root.py

djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 13)
The issue has been resolved. Show all issues
```
 'SpyAgency' imported but unused
```

Description:

		This project adds uri templates for extensions to the api. Presently,
~		The `RootResource` web API resource generates URI templates as
	~	the `RootResource` web API resource generates URI templates as
		shortcuts to users. Extensions are able to provide their own API
		resources, however they are not currently picked up by RootResource.
		It has been confirmed that this isn't a caching bug. Instead,
		RootResource needs to be modified to continue walking the tree upon
		encountering an extension. Currently, the proposed solution is to
		create a mixin class incorporating functionality from RootResource
		to track signals from the ExtensionResource. The ReviewBoard project
		RootResource would inherit this new mixin accordingly.

Testing Done:

~		Performed `http :8080/api/` request when no extensions are installed
~		and noted the absence of any extension templates in the response.
	~	Performed an HTTP request on `localhost:8080/api/` when no extensions are
	~	installed and noted the absence of any extension templates in the response.

		Installed the checklist extension from rb-extension-pack and enabled
~		it on my local instance. Made a new `http :8080/api/` request and
~		noted that the extension templates are now correctly included.
	~	it on my local instance. Made a new HTTP request to `localhost:8080/api/`
	~	and noted that the extension templates are now correctly included.
		Disabling the extension removes the extensions.

		Ran djblets unit tests and noted all tests pass.

		Adding new unit tests to verify functionality. All pass.

Description:

		This project adds uri templates for extensions to the api. Presently,
~		the `RootResource` web API resource generates URI templates as
	~	the `RootResource` web API resource generates URI templates as
		shortcuts to users. Extensions are able to provide their own API
		resources, however they are not currently picked up by RootResource.
		It has been confirmed that this isn't a caching bug. Instead,
		RootResource needs to be modified to continue walking the tree upon
		encountering an extension. Currently, the proposed solution is to
		create a mixin class incorporating functionality from RootResource
		to track signals from the ExtensionResource. The ReviewBoard project
		RootResource would inherit this new mixin accordingly.

Commit:

-	41551cece689f1b80529c221259796c7512392a3
+	d6c3edd5c20feb1ff8c3028dbc6d955b88f22589

Diff:

Revision 15 (+353 -10)

Show changes

	djblets/extensions/resources.py
	djblets/webapi/resources/root.py
	djblets/webapi/tests/test_extension_resource_mixin.py

Tool: Pyflakes
Processed Files:
    djblets/extensions/resources.py
    djblets/webapi/tests/test_extension_resource_mixin.py
    djblets/webapi/resources/root.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/extensions/resources.py
    djblets/webapi/tests/test_extension_resource_mixin.py
    djblets/webapi/resources/root.py

Commit:

-	d6c3edd5c20feb1ff8c3028dbc6d955b88f22589
+	9863db77748abc767782505b369521e914cb5bce

Diff:

Revision 16 (+356 -10)

Show changes

	djblets/extensions/resources.py
	djblets/webapi/resources/root.py
	djblets/webapi/tests/test_extension_resource_mixin.py

Tool: Pyflakes
Processed Files:
    djblets/extensions/resources.py
    djblets/webapi/tests/test_extension_resource_mixin.py
    djblets/webapi/resources/root.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/extensions/resources.py
    djblets/webapi/tests/test_extension_resource_mixin.py
    djblets/webapi/resources/root.py

djblets/extensions/resources.py (Diff revisions 14 - 16)
The issue has been resolved. Show all issues
```
This will fit on one line.
```

djblets/extensions/resources.py (Diff revisions 14 - 16)

The issue has been resolved. Show all issues

Reformat as:

                self.register_uri_template(entry.name, entry.list_href,
                                           ext_resource)

djblets/extensions/resources.py (Diff revisions 14 - 16)
The issue has been resolved. Show all issues
```
This will fit on one line
```
djblets/extensions/resources.py (Diff revisions 14 - 16)
The issue has been resolved. Show all issues
```
This will fit on one line.
```
djblets/webapi/resources/root.py (Diff revisions 14 - 16)
The issue has been resolved. Show all issues
```
djblets.webapi.resources.base.WebAPIResource
```
djblets/webapi/resources/root.py (Diff revisions 14 - 16)
The issue has been resolved. Show all issues
```
"Whether or not the resource is a list resource."
```

djblets/webapi/resources/root.py (Diff revisions 14 - 16)

The issue has been resolved. Show all issues

Could you reformat this as:

templates[registered_name] = (
    '%s%s'
    % (href, registered_href)
)

djblets/webapi/resources/root.py (Diff revisions 14 - 16)

The issue has been resolved. Show all issues

Reformat as:

            yield self.ResourceEntry(resource.name, object_href, resource,
                                     False)

djblets/webapi/resources/root.py (Diff revisions 14 - 16)

The issue has been resolved. Show all issues

This can be simplified to:

            for entry in self.walk_resources(child, child_href):
                yield entry

djblets/webapi/resources/root.py (Diff revisions 14 - 16)

The issue has been resolved. Show all issues

Reformat as:

        self._registered_uri_templates[relative_resource][name] = \
           relative_path

Commit:

-	9863db77748abc767782505b369521e914cb5bce
+	8f1a3fc6de1966687428862a4f954d06202ca8d8

Diff:

Revision 17 (+353 -10)

Show changes

	djblets/extensions/resources.py
	djblets/webapi/resources/root.py
	djblets/webapi/tests/test_extension_resource_mixin.py

Tool: Pyflakes
Processed Files:
    djblets/extensions/resources.py
    djblets/webapi/tests/test_extension_resource_mixin.py
    djblets/webapi/resources/root.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/extensions/resources.py
    djblets/webapi/tests/test_extension_resource_mixin.py
    djblets/webapi/resources/root.py

Ship it!

```
This should be the last review pass!
```
djblets/extensions/resources.py (Diff revision 17)
The issue has been resolved. Show all issues
```
Can you make a note that the subclass that uses this mixin must implement this?
```

djblets/webapi/resources/root.py (Diff revision 17)

The issue has been resolved. Show all issues

How about:

for name, href in six.iteritems(unassigned_templates):
    # ....

djblets/webapi/resources/root.py (Diff revision 17)

The issue has been resolved. Show all issues


for entry in self.walk_resources(self, base_href):

djblets/webapi/resources/root.py (Diff revision 17)

The issue has been resolved. Show all issues

How about:
                if is_list:
                    list_templates = self._registered_uri_templates.get(
                        resource, {})

                    for name, href in six.iteritems(list_templates):
                        templates[name] = (
                            '%s%s'
                            % (entry.href, href)
                    )

djblets/webapi/resources/root.py (Diff revision 17)

The issue has been resolved. Show all issues

This can be simplified to

for entry in self.walk_resources(child, child_href):
    yield entry

djblets/webapi/resources/root.py (Diff revision 17)

The issue has been resolved. Show all issues


for entry in self.walk_resources(child, child_href):
    yield entry

Commit:

-	8f1a3fc6de1966687428862a4f954d06202ca8d8
+	058ece480e6f6be7ae2bacd898aa5bb4b7e4c3a0

Diff:

Revision 18 (+356 -10)

Show changes

	djblets/extensions/resources.py
	djblets/webapi/resources/root.py
	djblets/webapi/tests/test_extension_resource_mixin.py

Tool: Pyflakes
Processed Files:
    djblets/extensions/resources.py
    djblets/webapi/tests/test_extension_resource_mixin.py
    djblets/webapi/resources/root.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/extensions/resources.py
    djblets/webapi/tests/test_extension_resource_mixin.py
    djblets/webapi/resources/root.py

djblets/extensions/resources.py (Diff revision 18)

The issue has been resolved. Show all issues

This doesn't really describe this properly.

I would say it's a mixin for root resources that make use of ExtensionResource.

You should then flesh out the description in the docstring to explain why this exists, what it accomplishes.

djblets/extensions/resources.py (Diff revision 18)
The issue has been resolved. Show all issues
```
Blank line between these.
```
djblets/extensions/resources.py (Diff revision 18)
The issue has been resolved. Show all issues
```
This is missing **kwargs.
```
djblets/extensions/resources.py (Diff revision 18)
The issue has been resolved. Show all issues
```
The type needs to be a full class path.
```
djblets/extensions/resources.py (Diff revision 18)
The issue has been resolved. Show all issues
```
Same comments as above.
```

djblets/extensions/resources.py (Diff revision 18)

The issue has been resolved. Show all issues

This is missing a "Returns" block that states the return type and content.

djblets/webapi/resources/root.py (Diff revision 18)
The issue has been resolved. Show all issues
```
This should be on the same line.
```
djblets/webapi/resources/root.py (Diff revision 18)
The issue has been resolved. Show all issues
```
Same line.
```
djblets/webapi/resources/root.py (Diff revision 18)
The issue has been resolved. Show all issues
```
This can be one line.
```

djblets/webapi/resources/root.py (Diff revision 18)

The issue has been resolved. Show all issues

Indentation level for "Args" and "Yields" is wrong.

djblets/webapi/resources/root.py (Diff revision 18)

The issue has been resolved. Show all issues

ResourceEntry needs to:

1) Be on the next line
2) Be RootResource.ResourceEntry (these need to be either absolute paths, if outside the current module, or relative to the current module).
3) Have a suitable description.

djblets/webapi/resources/root.py (Diff revision 18)

The issue has been resolved. Show all issues

Let's construct all these with keyword arguments instead of positional arguments for the values.

djblets/webapi/resources/root.py (Diff revision 18)
The issue has been resolved. Show all issues
```
This should remain one statement.
```
djblets/webapi/resources/root.py (Diff revision 18)
The issue has been resolved. Show all issues
```
Same comment as above for keyword arguments.
```
djblets/webapi/resources/root.py (Diff revision 18)
The issue has been resolved. Show all issues
```
"URI"
```

djblets/webapi/resources/root.py (Diff revision 18)

The issue has been resolved. Show all issues

_registered_uri_templates and _uri_templates are implementation details. Docs should focus on a higher level saying what this does, not how it does it.

djblets/webapi/resources/root.py (Diff revision 18)

The issue has been resolved. Show all issues

This should say what it's relative to.

rswanson Oct. 27, 2016, 10:28 p.m.

I've updated it to read: The path of the API resource relative to its parent resources.. Does that seem more appropriate?

djblets/webapi/resources/root.py (Diff revision 18)

The issue has been resolved. Show all issues

Needs to be an absolute class path.
Should also include , optional after the type.

djblets/webapi/resources/root.py (Diff revision 18)

The issue has been resolved. Show all issues

You can simplify much of this and reduce dictionary accesses:

templates = self._registered_uri_templates.setdefault(relative_resource,
                                                      {})
templates[name] = relative_path

djblets/webapi/resources/root.py (Diff revision 18)

The issue has been resolved. Show all issues

These are pretty conflicting statements. The second overrides the first.

rswanson Oct. 27, 2016, 10:28 p.m.

You're right - the first statement is an artifact from MANY iterations ago.

djblets/webapi/resources/root.py (Diff revision 18)
The issue has been resolved. Show all issues
```
"URI"
```

djblets/webapi/resources/root.py (Diff revision 18)

The issue has been resolved. Show all issues

Same as above with the doc content. It should be higher level than what internal fields are being modified.

djblets/webapi/resources/root.py (Diff revision 18)
The issue has been resolved. Show all issues
```
Full class path and , optional.
```
djblets/webapi/resources/root.py (Diff revision 18)
The issue has been resolved. Show all issues
```
"URI"
```

djblets/webapi/resources/root.py (Diff revision 18)

The issue has been resolved. Show all issues

Better to do:

try:
    del self._registered_uri_templates[relative_resource][name]
except KeyError:
    pass


This does two things:
1) Doesn't fail (like the current code) if relative_resource isn't in the dictionary.

2) Saves a lookup (or two). We assume the lookups and deletion will work, and ignore if it doesn't, instead of checking up-front (often unnecessarily).

RS

rswanson Oct. 27, 2016, 10:28 p.m.
```
Ah, this is a much better solution.
```

djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 18)

The issue has been resolved. Show all issues

Blank line between these. Djblets is the current project, and so has its own import group.

rswanson Oct. 27, 2016, 10:28 p.m.

Whoops, looks like I removed a line by accident.

djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 18)
The issue has been resolved. Show all issues
```
These can be one import statement.
```
djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 18)
The issue has been resolved. Show all issues
```
Please use numbers instead of spelling them out for these classes.
```
djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 18)
The issue has been resolved. Show all issues
```
Missing a trailing period.
Same with the ones below.
```

djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 18)

The issue has been resolved. Show all issues

This isn't in the proper form. It should be:

"""
...

Returns
    djblets.extensions.resources.ExtensionResource
    <description here>
"""

That said, these are unit tests. They're not necessary here unless they're test functions or utility functions designed to automate parts of the testing.

rswanson Oct. 27, 2016, 10:28 p.m.

Makes sense. It was an earlier review point to add the docstring here. This is a utility function that we need to implement when using the mixin, so I'd say it's probably necessary. I'll add a description.

djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 18)
The issue has been resolved. Show all issues
```
This should probably be ExtensionRootResourceMixinTests.
```

djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 18)

The issue has been resolved. Show all issues

This should be done first, probably.

Blank line between it and the setters.

rswanson Oct. 28, 2016, 12:28 a.m.

Forgot to address this in the original path. I found that when running super(..).setUp() first, I ran in to issues where the extension resource wasn't correctly initialized on the TestRootResource. Doing the initialization at the end prevented this.

djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 18)

The issue has been resolved. Show all issues

So two things:
1) When testing a method, the docstring needs to list the class name and the proper name of the function (this is missing the _ at the beginning).

2) We don't want to necessarily test just this function, but rather "is this done when an extension is initialized." So you should test from the starting point of the signal.
With #2, you probably want this called: Testing ExtensionRootResourceMixin generates URI templates when extensions initialized
These comments apply below as well.

djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 18)

The issue has been resolved. Show all issues

This whole thing should be used inline below, like:

self.assertEqual(
    actual_result,
    {
        ...
    })

djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 18)

The issue has been resolved. Show all issues

Should just use assertEqual, not assert*Equal methods.

Also, flip these. Thing you're testing should be first. Expected value second.

Same with all other tests.

rswanson Oct. 27, 2016, 10:28 p.m.

Ok, I know for things like JUnit the convention is to have expected values first and actual values second. I can switch it.

djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 18)
The issue has been resolved. Show all issues
```
This should also be inline below.
Same with other tests.
```
djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 18)
The issue has been resolved. Show all issues
```
This should be RootResourceTests, and should live in its own file (test_root_resource.py).
```

djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 18)

The issue has been resolved. Show all issues

Consistency is good. Both of these should look the same:

.... = {
    self.ext_resource: {
        'extensions': ...,
    },
    None: {
        'extensions': ...,
    },
}

djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 18)

The issue has been resolved. Show all issues

This can be one line.
Should ideally use keyword arguments, which would push to two lines, but they should start immediately after the ( in the function call.

djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 18)
The issue has been resolved. Show all issues
```
This can be one line.
```
djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 18)
The issue has been resolved. Show all issues
```
These calls will read better if you use keyword arguments.
```
djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 18)
The issue has been resolved. Show all issues
```
Should be inline.
```
djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 18)
The issue has been resolved. Show all issues
```
Condense this.
```

Commit:

-	058ece480e6f6be7ae2bacd898aa5bb4b7e4c3a0
+	8ca21fc1856903506de75c459bdf612db9a65bdb

Diff:

Revision 19 (+374 -10)

Show changes

	djblets/extensions/resources.py
	djblets/webapi/resources/root.py
	djblets/webapi/tests/test_extension_resource_mixin.py
	djblets/webapi/tests/test_root_resource.py

Tool: Pyflakes
Processed Files:
    djblets/webapi/tests/test_root_resource.py
    djblets/extensions/resources.py
    djblets/webapi/tests/test_extension_resource_mixin.py
    djblets/webapi/resources/root.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/webapi/tests/test_root_resource.py
    djblets/extensions/resources.py
    djblets/webapi/tests/test_extension_resource_mixin.py
    djblets/webapi/resources/root.py

djblets/webapi/resources/root.py (Diff revision 19)
The issue has been resolved. Show all issues
```
Col: 50
 E225 missing whitespace around operator
```
djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 19)
The issue has been resolved. Show all issues
```
Col: 9
 E303 too many blank lines (2)
```

Commit:

-	8ca21fc1856903506de75c459bdf612db9a65bdb
+	395dead3f8bdedad66396b7803a422a14ba49aaf

Diff:

Revision 20 (+372 -10)

Show changes

	djblets/extensions/resources.py
	djblets/webapi/resources/root.py
	djblets/webapi/tests/test_extension_resource_mixin.py
	djblets/webapi/tests/test_root_resource.py

Tool: Pyflakes
Processed Files:
    djblets/webapi/tests/test_root_resource.py
    djblets/extensions/resources.py
    djblets/webapi/tests/test_extension_resource_mixin.py
    djblets/webapi/resources/root.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/webapi/tests/test_root_resource.py
    djblets/extensions/resources.py
    djblets/webapi/tests/test_extension_resource_mixin.py
    djblets/webapi/resources/root.py

The issue has been resolved. Show all issues
```
"URI" in your summary.
```

djblets/extensions/resources.py (Diff revision 20)

The issue has been dropped. Show all issues

This is not a class, it is an instance (isn't it?) Rename this to ext or extension.
If it is a class then you want to use type and mention it must be a subclass of djblets.extensions.Extension.

rswanson Oct. 29, 2016, 2:33 p.m.

It's an instance - however the name has been determined by the listener.

djblets/extensions/resources.py (Diff revision 20)
The issue has been resolved. Show all issues
```
"the URI templates."
```
djblets/extensions/resources.py (Diff revision 20)
The issue has been dropped. Show all issues
```
Same here.
```
djblets/extensions/resources.py (Diff revision 20)
The issue has been resolved. Show all issues
```
"the URI templates."
```
djblets/webapi/resources/root.py (Diff revision 20)
The issue has been resolved. Show all issues
```
Blank line between these.
```

djblets/webapi/resources/root.py (Diff revision 20)

The issue has been resolved. Show all issues

Can you change this to be a @classmethod ?
e.g.

@classmethod
def walk_resources(cls, resource, list_href):


and replace all usages of self with cls ?

djblets/webapi/resources/root.py (Diff revision 20)
The issue has been resolved. Show all issues
```
Please elaborate on this.
```
djblets/webapi/resources/root.py (Diff revision 20)
The issue has been resolved. Show all issues
```
Don't wrap this. You will get a error from review bot but you can drop it.
```
djblets/webapi/tests/test_root_resource.py (Diff revision 20)
The issue has been resolved. Show all issues
```
Blank line between these
```

Summary:

-	Providing uri templates for extensions
+	Providing URI templates for extensions

Commit:

-	395dead3f8bdedad66396b7803a422a14ba49aaf
+	fa991b256d2bb93295d4d2385fc1f451d8bb8d11

Diff:

Revision 21 (+377 -10)

Show changes

	djblets/extensions/resources.py
	djblets/webapi/resources/root.py
	djblets/webapi/tests/test_extension_resource_mixin.py
	djblets/webapi/tests/test_root_resource.py

Tool: Pyflakes
Processed Files:
    djblets/webapi/tests/test_root_resource.py
    djblets/extensions/resources.py
    djblets/webapi/tests/test_extension_resource_mixin.py
    djblets/webapi/resources/root.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/webapi/tests/test_root_resource.py
    djblets/extensions/resources.py
    djblets/webapi/tests/test_extension_resource_mixin.py
    djblets/webapi/resources/root.py

djblets/webapi/resources/root.py (Diff revision 21)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (89 > 79 characters)
```
djblets/webapi/resources/root.py (Diff revision 21)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (89 > 79 characters)
```

The issue has been resolved. Show all issues

For the Description, "ReviewBoard" -> "Review Board".

djblets/extensions/resources.py (Diff revision 21)

The issue has been dropped. Show all issues

I actually don't think we normally document *args and **kwargs, since I often think that it's for passing through arguments as opposed to doing something with them.

Like, in this case, __init__ is not doing anything with either *args or **kwargs.

Same goes for the other documentation of *args and **kwargs in this file.

If I'm contradicting feedback from another mentor, ignore me - but I've been looking around, and I just don't see documentation for these things in our tree, and I suspect that's on purpose.

rswanson Nov. 4, 2016, 9:07 p.m.

The documentation request came from another mentor, so I'll keep it as-is for now if that's alright.

djblets/webapi/resources/root.py (Diff revision 21)

The issue has been dropped. Show all issues

This is an interesting documentation style choice, the #:. Can you explain why you made that choice? I'm not familiar with that style.

rswanson Nov. 4, 2016, 9:07 p.m.

Again, this documentation style for a namedtuple came as a suggestion from a mentor.

brennie Nov. 7, 2016, 9:20 p.m.

Attributes and data commented with #: will be picked up by sphinx as if it were a doc comment for that data/attribute.

djblets/webapi/resources/root.py (Diff revision 21)
The issue has been resolved. Show all issues
```
Leftovers?
```
1. RS
  
  rswanson Nov. 4, 2016, 9:07 p.m.
  Oops. Yeah.

Description:

		This project adds uri templates for extensions to the api. Presently,
		the `RootResource` web API resource generates URI templates as
		shortcuts to users. Extensions are able to provide their own API
		resources, however they are not currently picked up by RootResource.
		It has been confirmed that this isn't a caching bug. Instead,
		RootResource needs to be modified to continue walking the tree upon
		encountering an extension. Currently, the proposed solution is to
		create a mixin class incorporating functionality from RootResource
~		to track signals from the ExtensionResource. The ReviewBoard project
	~	to track signals from the ExtensionResource. The Review Board project
		RootResource would inherit this new mixin accordingly.

Commit:

-	fa991b256d2bb93295d4d2385fc1f451d8bb8d11
+	93c41f67e740ae710d5d27e686c8441ccafeff6d

Diff:

Revision 22 (+375 -10)

Show changes

	djblets/extensions/resources.py
	djblets/webapi/resources/root.py
	djblets/webapi/tests/test_extension_resource_mixin.py
	djblets/webapi/tests/test_root_resource.py

Tool: PEP8 Style Checker
Processed Files:
    djblets/webapi/tests/test_root_resource.py
    djblets/extensions/resources.py
    djblets/webapi/tests/test_extension_resource_mixin.py
    djblets/webapi/resources/root.py



Tool: Pyflakes
Processed Files:
    djblets/webapi/tests/test_root_resource.py
    djblets/extensions/resources.py
    djblets/webapi/tests/test_extension_resource_mixin.py
    djblets/webapi/resources/root.py

djblets/webapi/resources/root.py (Diff revision 22)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (89 > 79 characters)
```
djblets/webapi/resources/root.py (Diff revision 22)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (89 > 79 characters)
```

Almost at the finish line! Everything here is either a tiny doc fix or a tiny syntax/consistency issue.

The issue has been resolved. Show all issues

test_root_resource.py is missing a module docstring.

djblets/extensions/resources.py (Diff revision 22)

The issue has been resolved. Show all issues

This (and all other references to djblets.extensions.Extension) needs to be djblets.extensions.extension.Extension). The former doesn't exist, and only the latter will properly generate links in the docs.

djblets/extensions/resources.py (Diff revision 22)

The issue has been resolved. Show all issues

Public methods must come before private method. This should be right after __init__.

djblets/extensions/resources.py (Diff revision 22)
The issue has been resolved. Show all issues
```
No parens.
```
djblets/webapi/resources/root.py (Diff revision 22)
The issue has been resolved. Show all issues
```
No outer parens needed.
```
djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 22)
The issue has been resolved. Show all issues
```
No parens.
```

Commit:

-	93c41f67e740ae710d5d27e686c8441ccafeff6d
+	f8d57ab01ce62be5dd2d04fc746dd328c976d825

Diff:

Revision 23 (+377 -10)

Show changes

	djblets/extensions/resources.py
	djblets/webapi/resources/root.py
	djblets/webapi/tests/test_extension_resource_mixin.py
	djblets/webapi/tests/test_root_resource.py

Tool: Pyflakes
Processed Files:
    djblets/webapi/tests/test_root_resource.py
    djblets/extensions/resources.py
    djblets/webapi/tests/test_extension_resource_mixin.py
    djblets/webapi/resources/root.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/webapi/tests/test_root_resource.py
    djblets/extensions/resources.py
    djblets/webapi/tests/test_extension_resource_mixin.py
    djblets/webapi/resources/root.py

djblets/webapi/resources/root.py (Diff revision 23)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (89 > 79 characters)
```
djblets/webapi/resources/root.py (Diff revision 23)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (89 > 79 characters)
```

This looks great! I just have a couple minor things left, and then I think we're ready to land this :)

djblets/webapi/resources/root.py (Diff revision 23)
The issue has been resolved. Show all issues
```
This is indented one level too far.
```

djblets/webapi/resources/root.py (Diff revision 23)

The issue has been resolved. Show all issues

Can you add a comment here just saying that we're clearing the cache so that future lookups will take into account the new template? Just helps make it clear what's going on there.

djblets/webapi/resources/root.py (Diff revision 23)
The issue has been resolved. Show all issues
```
Same comment here as above with a comment.
```

Commit:

-	f8d57ab01ce62be5dd2d04fc746dd328c976d825
+	6fb2eed19d50533d541189a3f4a2a6329440bdf3

Diff:

Revision 24 (+379 -10)

Show changes

	djblets/extensions/resources.py
	djblets/webapi/resources/root.py
	djblets/webapi/tests/test_extension_resource_mixin.py
	djblets/webapi/tests/test_root_resource.py

Tool: Pyflakes
Processed Files:
    djblets/webapi/tests/test_root_resource.py
    djblets/extensions/resources.py
    djblets/webapi/tests/test_extension_resource_mixin.py
    djblets/webapi/resources/root.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/webapi/tests/test_root_resource.py
    djblets/extensions/resources.py
    djblets/webapi/tests/test_extension_resource_mixin.py
    djblets/webapi/resources/root.py

djblets/webapi/resources/root.py (Diff revision 24)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (89 > 79 characters)
```
djblets/webapi/resources/root.py (Diff revision 24)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (89 > 79 characters)
```

djblets/webapi/resources/root.py (Diff revision 24)

Is there special meaning in having #:? It also seems somewhat inconsistent (eg. the line with list_href doesn't have it).

rswanson Nov. 24, 2016, 9:26 a.m.

Mike actually asked the same question! This was a suggestion from another mentor that I'd incorporated in to the code, so I kept their comment choice the same for simplicity's sake.

Ship it!

```
Two TINY nits. This looks good to me!
```
djblets/extensions/resources.py (Diff revision 24)
The issue has been resolved. Show all issues
```
This doesn't need to be a raw string.
```
djblets/extensions/resources.py (Diff revision 24)
The issue has been resolved. Show all issues
```
Again, this doesn't need to be a raw string.
```

Commit:

-	6fb2eed19d50533d541189a3f4a2a6329440bdf3
+	c4ef69eb101cab23f8ea5ff8bb8e24694d25c1f8

Diff:

Revision 25 (+379 -10)

Show changes

	djblets/extensions/resources.py
	djblets/webapi/resources/root.py
	djblets/webapi/tests/test_extension_resource_mixin.py
	djblets/webapi/tests/test_root_resource.py

Tool: Pyflakes
Processed Files:
    djblets/webapi/tests/test_root_resource.py
    djblets/extensions/resources.py
    djblets/webapi/tests/test_extension_resource_mixin.py
    djblets/webapi/resources/root.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/webapi/tests/test_root_resource.py
    djblets/extensions/resources.py
    djblets/webapi/tests/test_extension_resource_mixin.py
    djblets/webapi/resources/root.py

djblets/webapi/resources/root.py (Diff revision 25)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (89 > 79 characters)
```
djblets/webapi/resources/root.py (Diff revision 25)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (89 > 79 characters)
```

Ship it!

djblets/webapi/resources/root.py (Diff revisions 23 - 25)
The issue has been resolved. Show all issues
```
Missing trailing period. All comments should be in full sentence format.
```
djblets/webapi/resources/root.py (Diff revisions 23 - 25)
The issue has been resolved. Show all issues
```
Same here.
```

Change Summary:

Handing this off to Maram.

Owner:

-	rswanson
+	maram

Change Summary:

Added missing trailing periods. Re-ran unit tests, all okay.

Branch:

-	release-0.10.x
+	master

Commit:

-	c4ef69eb101cab23f8ea5ff8bb8e24694d25c1f8
+	5e639a88dadbef8d33f8d5a453d787fb5d353a16

Diff:

Revision 26 (+379 -10)

Show changes

	djblets/extensions/resources.py
	djblets/webapi/resources/root.py
	djblets/webapi/tests/test_extension_resource_mixin.py
	djblets/webapi/tests/test_root_resource.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

djblets/webapi/resources/root.py (Diff revision 26)
The issue has been resolved. Show all issues
```
E501 line too long (89 > 79 characters)
```
djblets/webapi/resources/root.py (Diff revision 26)
The issue has been resolved. Show all issues
```
E501 line too long (89 > 79 characters)
```

Commit:

-	5e639a88dadbef8d33f8d5a453d787fb5d353a16
+	3ef58d36995c7e3e2cb719f699cbbf74c03739ad

Diff:

Revision 27 (+381 -10)

Show changes

	djblets/extensions/resources.py
	djblets/webapi/resources/root.py
	djblets/webapi/tests/test_extension_resource_mixin.py
	djblets/webapi/tests/test_root_resource.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (d84cc41)