Providing URI templates for extensions

Review Request #8432 — Created Sept. 24, 2016 and submitted

Information

Djblets
master
3ef58d3...

Reviewers

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.

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.

Description From Last Updated

test_extensionresourcemixin.py should be test_extension_resource_mixin.py.

brenniebrennie

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

brenniebrennie

I noticed your testing done mentions the httpie tool and the fact that the devserver is on port 8080. Can …

brenniebrennie

"URI" in your summary.

brenniebrennie

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

mike_conleymike_conley

test_root_resource.py is missing a module docstring.

chipx86chipx86

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 80 E501 line too long (82 > 79 characters)

reviewbotreviewbot

Col: 1 W391 blank line at end of file

reviewbotreviewbot

Col: 69 E261 at least two spaces before inline comment

reviewbotreviewbot

Col: 80 E501 line too long (88 > 79 characters)

reviewbotreviewbot

Col: 59 W292 no newline at end of file

reviewbotreviewbot

Col: 19 W292 no newline at end of file

reviewbotreviewbot

Col: 80 E501 line too long (81 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (82 > 79 characters)

reviewbotreviewbot

This will save you a lot of review changes. Blank line around blocks (like if, for, etc.).

LA larmiej

Module imports go above symbol imports, so this should be like: import six from djblets.foo import bar...

brenniebrennie

Missing a period.

brenniebrennie

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

brenniebrennie

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

The comment should be a docstring.

brenniebrennie

Missing docstring.

brenniebrennie

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Mixins should inherit from object, otherwise it's just a subclass. The idea is that a mixin is a very small, …

chipx86chipx86

I know this is a WIP change, but for here and other docstrings, make sure to follow the requirements in …

chipx86chipx86

This should be at the top of the file.

chipx86chipx86

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

chipx86chipx86

This should just be a keyword argument in the method definition. Same further down.

chipx86chipx86

Blank line between these (between code and blocks like if/for).

chipx86chipx86

We use Django's version of six: from django.utils import six Note also that six (and Django's version) is a third-party …

chipx86chipx86

Should be 2 blank lines.

chipx86chipx86

Yep, this will need to be extracted, but there's another thing that's important to note as well. As currently written, …

chipx86chipx86

Col: 49 E127 continuation line over-indented for visual indent

reviewbotreviewbot

__init__ for a subclass/mixin MUST call the superclass __init__ method, in this case: super(ExtensionRootResourceMixin, self).__init__()

brenniebrennie

Col: 80 E501 line too long (85 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (81 > 79 characters)

reviewbotreviewbot

Col: 33 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 71 W292 no newline at end of file

reviewbotreviewbot

Col: 49 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 80 E501 line too long (85 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (81 > 79 characters)

reviewbotreviewbot

Col: 33 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 71 W292 no newline at end of file

reviewbotreviewbot

Col: 49 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 80 E501 line too long (85 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (81 > 79 characters)

reviewbotreviewbot

'ipdb' imported but unused

reviewbotreviewbot

Col: 80 E501 line too long (88 > 79 characters)

reviewbotreviewbot

Col: 56 E261 at least two spaces before inline comment

reviewbotreviewbot

Col: 33 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 71 W292 no newline at end of file

reviewbotreviewbot

Col: 49 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 80 E501 line too long (85 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (81 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

Col: 51 E261 at least two spaces before inline comment

reviewbotreviewbot

Col: 33 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 12 E713 test for membership should be 'not in'

reviewbotreviewbot

Col: 12 E713 test for membership should be 'not in'

reviewbotreviewbot

Col: 49 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 80 E501 line too long (85 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (81 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

Col: 51 E261 at least two spaces before inline comment

reviewbotreviewbot

Col: 33 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

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

brenniebrennie

Missing docstring.

brenniebrennie

I don't think this tells us anything.

brenniebrennie

No blank line here.

brenniebrennie

This is pretty unweildly. I think it would be better to pull this out into a variable and iterate over …

brenniebrennie

This should be '%s%s' % (href, registered_href). Generally, you'll want to avoid string concatenation in Python because it is slow. …

brenniebrennie

"Yield" Docstrings for methods should always be written in the imperative mood, i.e., they tell (or command) you what to …

brenniebrennie

Missing Args and Yields Yields is just like Returns.

brenniebrennie

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

brenniebrennie

Missing Args.

brenniebrennie

Here, too.

brenniebrennie

The first import should be: from __future__ import unicode_literals This turns all 'string's into u'string's automatically. The str (default for …

brenniebrennie

'ExtensionResource' imported but unused

reviewbotreviewbot

Docstring.

brenniebrennie

Missing tests.

brenniebrennie

Docstring. This should be something like """Unit tests for root resource template registration."""

brenniebrennie

Please don't use mock.Mock. Create a dummy class that has all the stuff you need. mock is a huge dependency …

brenniebrennie

Col: 13 E122 continuation line missing indentation or outdented

reviewbotreviewbot

Col: 80 E501 line too long (89 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (91 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (85 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (91 > 79 characters)

reviewbotreviewbot

"URI template cache"

brenniebrennie

Col: 80 E501 line too long (92 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (92 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (89 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (92 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (92 > 79 characters)

reviewbotreviewbot

Col: 68 W292 no newline at end of file

reviewbotreviewbot

Additionally, docstrings should be of the form: """Single line summary. Multi-line description. """ You may want to cut out the …

brenniebrennie

This should line up with the first ".

brenniebrennie

Same here.

brenniebrennie

Docstrings should be in the imperitive moode ("Generate"). Missing Args, Returns blocks

brenniebrennie

And here, too.

brenniebrennie

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

brenniebrennie

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

brenniebrennie

Backslashes are icky and can lead to bugs when there is trailing whitespace that accidentally committed, so we tend to …

brenniebrennie

unicode.

brenniebrennie

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

brenniebrennie

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

brenniebrennie

Pull this out, too.

brenniebrennie

Imperitive mood ("Register")

brenniebrennie

unicode.

brenniebrennie

unicode

brenniebrennie

"Unregister" This may not be a word, but what do dictionaries know? They let frobnicate be a word :)

brenniebrennie

unicode

brenniebrennie

Not really necessary tbh.

brenniebrennie

"""Unit tests for the ExtensionRootResourceMixin.""" No need for full class paths here.

brenniebrennie

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

brenniebrennie

Docstrings should be of the form: """Single line summary. Multi-line description. """ See my comment on the ExtensionTemplateTests for what …

brenniebrennie

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

brenniebrennie

No blank line here.

brenniebrennie

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

brenniebrennie

Col: 13 E122 continuation line missing indentation or outdented

reviewbotreviewbot

Col: 80 E501 line too long (89 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (91 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (85 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (91 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (92 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (92 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (89 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (92 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (92 > 79 characters)

reviewbotreviewbot

Col: 55 E201 whitespace after '('

reviewbotreviewbot

Col: 51 E202 whitespace before ')'

reviewbotreviewbot

'SpyAgency' imported but unused

reviewbotreviewbot

Col: 80 E501 line too long (87 > 79 characters)

reviewbotreviewbot

Col: 30 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Blank line between these.

brenniebrennie

Missing Args

brenniebrennie

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

brenniebrennie

Since this isnt a type object, can we rename this to ext or extension?

brenniebrennie

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, …

brenniebrennie

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

brenniebrennie

Same here re: kwargs.

brenniebrennie

Same here.

brenniebrennie

This will fit on one line.

brenniebrennie

Reformat as: self.register_uri_template(entry.name, entry.list_href, ext_resource)

brenniebrennie

Same thing here.

brenniebrennie

Here, too.

brenniebrennie

This will fit on one line

brenniebrennie

This will fit on one line.

brenniebrennie

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

brenniebrennie

"Attributes".

brenniebrennie

Can you reformat this as: #: Attributes #: field (type): #: Description of field... #: #: field (type): #: Description …

brenniebrennie

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

brenniebrennie

djblets.webapi.resources.base.WebAPIResource

brenniebrennie

"Whether or not the resource is a list resource."

brenniebrennie

Col: 55 E201 whitespace after '('

reviewbotreviewbot

Col: 51 E202 whitespace before ')'

reviewbotreviewbot

Blank line between these.

brenniebrennie

Could you reformat this as: templates[registered_name] = ( '%s%s' % (href, registered_href) )

brenniebrennie

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

brenniebrennie

This should yield self.ResourceEntry(name, list_href, resource, is_list).

brenniebrennie

And here

brenniebrennie

And here.

brenniebrennie

Reformat as: yield self.ResourceEntry(resource.name, object_href, resource, False)

brenniebrennie

This can be simplified to: for entry in self.walk_resources(child, child_href): yield entry

brenniebrennie

Can we rename this relative_path

brenniebrennie

Can we name this relative_resource or parent_resource?

brenniebrennie

Blank line between these.

brenniebrennie

Again, can we rename this relative_resource or parent_resource?

brenniebrennie

Reformat as: self._registered_uri_templates[relative_resource][name] = \ relative_path

brenniebrennie

Docstrings for all these

brenniebrennie

Can we call this TestChildResourceOne

brenniebrennie

TestChildResourceTwo

brenniebrennie

TestResourceOne

brenniebrennie

TestResourceTwo

brenniebrennie

TestExtension

brenniebrennie

TestRootResource

brenniebrennie

Docstrings should be of the form """Single line summary. Multi-line description. """

brenniebrennie

Rename to expected_result, actual_result. Also, I think this would be better formatted as the following: expected_result = { 'extension': 'http://testserver/{extension_name}/', …

brenniebrennie

Col: 80 E501 line too long (81 > 79 characters)

reviewbotreviewbot

Add a trailing comma.

brenniebrennie

Col: 80 E501 line too long (81 > 79 characters)

reviewbotreviewbot

Docstrings should be of the form """Single line summary. Multi-line description. """

brenniebrennie

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])

brenniebrennie

Can we just call these .ext_mgr and .extension?

brenniebrennie

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 …

brenniebrennie

Rename as actual_result, expected_result ?

brenniebrennie

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

brenniebrennie

Rename as expected_result, actual_result?

brenniebrennie

This should be formatted as: self.assertFalse(self.root_resource._registered_uri_tempaltes[ self.mock_extension])

brenniebrennie

Col: 30 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

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

brenniebrennie

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

brenniebrennie

Can you make a note that the subclass that uses this mixin must implement this?

brenniebrennie

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

brenniebrennie

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

brenniebrennie

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, …

brenniebrennie

This can be simplified to for entry in self.walk_resources(child, child_href): yield entry

brenniebrennie

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

brenniebrennie

This doesn't really describe this properly. I would say it's a mixin for root resources that make use of ExtensionResource. …

chipx86chipx86

Blank line between these.

chipx86chipx86

This is missing **kwargs.

chipx86chipx86

The type needs to be a full class path.

chipx86chipx86

Same comments as above.

chipx86chipx86

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

chipx86chipx86

This should be on the same line.

chipx86chipx86

Same line.

chipx86chipx86

This can be one line.

chipx86chipx86

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

chipx86chipx86

ResourceEntry needs to: 1) Be on the next line 2) Be RootResource.ResourceEntry (these need to be either absolute paths, if …

chipx86chipx86

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

chipx86chipx86

This should remain one statement.

chipx86chipx86

Same comment as above for keyword arguments.

chipx86chipx86

"URI"

chipx86chipx86

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

chipx86chipx86

This should say what it's relative to.

chipx86chipx86

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

chipx86chipx86

You can simplify much of this and reduce dictionary accesses: templates = self._registered_uri_templates.setdefault(relative_resource, {}) templates[name] = relative_path

chipx86chipx86

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

chipx86chipx86

"URI"

chipx86chipx86

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

chipx86chipx86

Full class path and , optional.

chipx86chipx86

"URI"

chipx86chipx86

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) …

chipx86chipx86

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

chipx86chipx86

These can be one import statement.

chipx86chipx86

Please use numbers instead of spelling them out for these classes.

chipx86chipx86

Missing a trailing period. Same with the ones below.

chipx86chipx86

This isn't in the proper form. It should be: """ ... Returns djblets.extensions.resources.ExtensionResource <description here> """ That said, these are …

chipx86chipx86

This should probably be ExtensionRootResourceMixinTests.

chipx86chipx86

This should be done first, probably. Blank line between it and the setters.

chipx86chipx86

So two things: 1) When testing a method, the docstring needs to list the class name and the proper name …

chipx86chipx86

This whole thing should be used inline below, like: self.assertEqual( actual_result, { ... })

chipx86chipx86

Should just use assertEqual, not assert*Equal methods. Also, flip these. Thing you're testing should be first. Expected value second. Same …

chipx86chipx86

This should also be inline below. Same with other tests.

chipx86chipx86

This should be RootResourceTests, and should live in its own file (test_root_resource.py).

chipx86chipx86

Consistency is good. Both of these should look the same: .... = { self.ext_resource: { 'extensions': ..., }, None: { …

chipx86chipx86

This can be one line. Should ideally use keyword arguments, which would push to two lines, but they should start …

chipx86chipx86

This can be one line.

chipx86chipx86

These calls will read better if you use keyword arguments.

chipx86chipx86

Should be inline.

chipx86chipx86

Condense this.

chipx86chipx86

Col: 50 E225 missing whitespace around operator

reviewbotreviewbot

Col: 9 E303 too many blank lines (2)

reviewbotreviewbot

This is not a class, it is an instance (isn't it?) Rename this to ext or extension. If it is …

brenniebrennie

"the URI templates."

brenniebrennie

Same here.

brenniebrennie

"the URI templates."

brenniebrennie

Blank line between these.

brenniebrennie

Can you change this to be a @classmethod ? e.g. @classmethod def walk_resources(cls, resource, list_href): and replace all usages of …

brenniebrennie

Please elaborate on this.

brenniebrennie

Don't wrap this. You will get a error from review bot but you can drop it.

brenniebrennie

Blank line between these

brenniebrennie

I actually don't think we normally document *args and **kwargs, since I often think that it's for passing through arguments …

mike_conleymike_conley

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

mike_conleymike_conley

Leftovers?

mike_conleymike_conley

Col: 80 E501 line too long (89 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (89 > 79 characters)

reviewbotreviewbot

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 …

chipx86chipx86

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

chipx86chipx86

No parens.

chipx86chipx86

No outer parens needed.

chipx86chipx86

Col: 80 E501 line too long (89 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (89 > 79 characters)

reviewbotreviewbot

No parens.

chipx86chipx86

This is indented one level too far.

chipx86chipx86

Col: 80 E501 line too long (89 > 79 characters)

reviewbotreviewbot

Can you add a comment here just saying that we're clearing the cache so that future lookups will take into …

chipx86chipx86

Missing trailing period. All comments should be in full sentence format.

chipx86chipx86

Col: 80 E501 line too long (89 > 79 characters)

reviewbotreviewbot

Same comment here as above with a comment.

chipx86chipx86

Same here.

chipx86chipx86

This doesn't need to be a raw string.

brenniebrennie

Again, this doesn't need to be a raw string.

brenniebrennie

Col: 80 E501 line too long (89 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (89 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (89 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (89 > 79 characters)

reviewbotreviewbot

E501 line too long (89 > 79 characters)

reviewbotreviewbot

E501 line too long (89 > 79 characters)

reviewbotreviewbot
reviewbot
  1. 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
    
    
  2. Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  3. Show all issues
    Col: 80
     E501 line too long (82 > 79 characters)
    
  4. Show all issues
    Col: 1
     W391 blank line at end of file
    
  5. djblets/webapi/resources/root.py (Diff revision 1)
     
     
    Show all issues
    Col: 69
     E261 at least two spaces before inline comment
    
  6. djblets/webapi/resources/root.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (88 > 79 characters)
    
  7. djblets/webapi/resources/root.py (Diff revision 1)
     
     
    Show all issues
    Col: 59
     W292 no newline at end of file
    
  8. 
      
RS
reviewbot
  1. 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
    
    
  2. Show all issues
    Col: 19
     W292 no newline at end of file
    
  3. djblets/webapi/resources/root.py (Diff revision 2)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  4. djblets/webapi/resources/root.py (Diff revision 2)
     
     
    Show all issues
    Col: 80
     E501 line too long (82 > 79 characters)
    
  5. 
      
RS
LA
  1. 
      
  2. djblets/webapi/resources/root.py (Diff revision 2)
     
     
    Show all issues

    This will save you a lot of review changes.

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

  3. 
      
RS
reviewbot
  1. 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
    
    
  2. djblets/webapi/resources/root.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (83 > 79 characters)
    
  3. 
      
brennie
  1. 
      
  2. djblets/webapi/resources/root.py (Diff revision 3)
     
     
    Show all issues

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

    import six
    from djblets.foo import bar...
    
  3. djblets/webapi/resources/root.py (Diff revision 3)
     
     
    Show all issues

    Missing a period.

  4. djblets/webapi/resources/root.py (Diff revision 3)
     
     
     
    Show all issues

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

    1. 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.

    2. 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.

  5. djblets/webapi/resources/root.py (Diff revision 3)
     
     
     
    Show all issues

    The comment should be a docstring.

  6. djblets/webapi/resources/root.py (Diff revision 3)
     
     
    Show all issues

    Missing docstring.

  7. 
      
RS
reviewbot
  1. 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
    
    
  2. djblets/extensions/resources.py (Diff revision 4)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  3. 
      
chipx86
  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.

  2. djblets/extensions/resources.py (Diff revision 4)
     
     
    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).

    1. 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.

  3. djblets/extensions/resources.py (Diff revision 4)
     
     
     
    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/

    1. I'll be sure to follow it!

  4. djblets/extensions/resources.py (Diff revision 4)
     
     
     
    Show all issues

    This should be at the top of the file.

  5. djblets/extensions/resources.py (Diff revision 4)
     
     
    Show all issues

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

  6. djblets/extensions/resources.py (Diff revision 4)
     
     
    Show all issues

    This should just be a keyword argument in the method definition. Same further down.

    1. TIL about keyword arguments!

  7. djblets/extensions/resources.py (Diff revision 4)
     
     
     
    Show all issues

    Blank line between these (between code and blocks like if/for).

  8. djblets/webapi/resources/root.py (Diff revision 4)
     
     
    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.

  9. djblets/webapi/resources/root.py (Diff revision 4)
     
     
     
     
     
     
    Show all issues

    Should be 2 blank lines.

  10. djblets/webapi/resources/root.py (Diff revision 4)
     
     
     
     
     
     
    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.

    1. 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.

    2. 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!

  11. 
      
RS
reviewbot
  1. 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
    
    
  2. djblets/extensions/resources.py (Diff revision 5)
     
     
    Show all issues
    Col: 49
     E127 continuation line over-indented for visual indent
    
  3. djblets/extensions/resources.py (Diff revision 5)
     
     
    Show all issues
    Col: 80
     E501 line too long (85 > 79 characters)
    
  4. djblets/extensions/resources.py (Diff revision 5)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  5. djblets/webapi/resources/root.py (Diff revision 5)
     
     
    Show all issues
    Col: 33
     E126 continuation line over-indented for hanging indent
    
  6. djblets/webapi/resources/root.py (Diff revision 5)
     
     
    Show all issues
    Col: 71
     W292 no newline at end of file
    
  7. 
      
brennie
  1. 
      
  2. djblets/extensions/resources.py (Diff revision 5)
     
     
    Show all issues

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

    super(ExtensionRootResourceMixin, self).__init__()
    
  3. 
      
RS
reviewbot
  1. 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
    
    
  2. djblets/extensions/resources.py (Diff revision 6)
     
     
    Show all issues
    Col: 49
     E127 continuation line over-indented for visual indent
    
  3. djblets/extensions/resources.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (85 > 79 characters)
    
  4. djblets/extensions/resources.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  5. djblets/webapi/resources/root.py (Diff revision 6)
     
     
    Show all issues
    Col: 33
     E126 continuation line over-indented for hanging indent
    
  6. djblets/webapi/resources/root.py (Diff revision 6)
     
     
    Show all issues
    Col: 71
     W292 no newline at end of file
    
  7. 
      
RS
reviewbot
  1. 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
    
    
  2. djblets/extensions/resources.py (Diff revision 7)
     
     
    Show all issues
    Col: 49
     E127 continuation line over-indented for visual indent
    
  3. djblets/extensions/resources.py (Diff revision 7)
     
     
    Show all issues
    Col: 80
     E501 line too long (85 > 79 characters)
    
  4. djblets/extensions/resources.py (Diff revision 7)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  5. djblets/webapi/resources/base.py (Diff revision 7)
     
     
    Show all issues
     'ipdb' imported but unused
    
  6. djblets/webapi/resources/base.py (Diff revision 7)
     
     
    Show all issues
    Col: 80
     E501 line too long (88 > 79 characters)
    
  7. djblets/webapi/resources/base.py (Diff revision 7)
     
     
    Show all issues
    Col: 56
     E261 at least two spaces before inline comment
    
  8. djblets/webapi/resources/root.py (Diff revision 7)
     
     
    Show all issues
    Col: 33
     E126 continuation line over-indented for hanging indent
    
  9. djblets/webapi/resources/root.py (Diff revision 7)
     
     
    Show all issues
    Col: 71
     W292 no newline at end of file
    
  10. 
      
RS
reviewbot
  1. 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
    
    
  2. djblets/extensions/resources.py (Diff revision 8)
     
     
    Show all issues
    Col: 49
     E127 continuation line over-indented for visual indent
    
  3. djblets/extensions/resources.py (Diff revision 8)
     
     
    Show all issues
    Col: 80
     E501 line too long (85 > 79 characters)
    
  4. djblets/extensions/resources.py (Diff revision 8)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  5. djblets/webapi/resources/base.py (Diff revision 8)
     
     
    Show all issues
    Col: 80
     E501 line too long (83 > 79 characters)
    
  6. djblets/webapi/resources/base.py (Diff revision 8)
     
     
    Show all issues
    Col: 51
     E261 at least two spaces before inline comment
    
  7. djblets/webapi/resources/root.py (Diff revision 8)
     
     
    Show all issues
    Col: 33
     E126 continuation line over-indented for hanging indent
    
  8. djblets/webapi/resources/root.py (Diff revision 8)
     
     
    Show all issues
    Col: 12
     E713 test for membership should be 'not in'
    
  9. djblets/webapi/resources/root.py (Diff revision 8)
     
     
    Show all issues
    Col: 12
     E713 test for membership should be 'not in'
    
  10. 
      
RS
reviewbot
  1. 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
    
    
  2. djblets/extensions/resources.py (Diff revision 9)
     
     
    Show all issues
    Col: 49
     E127 continuation line over-indented for visual indent
    
  3. djblets/extensions/resources.py (Diff revision 9)
     
     
    Show all issues
    Col: 80
     E501 line too long (85 > 79 characters)
    
  4. djblets/extensions/resources.py (Diff revision 9)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  5. djblets/webapi/resources/base.py (Diff revision 9)
     
     
    Show all issues
    Col: 80
     E501 line too long (83 > 79 characters)
    
  6. djblets/webapi/resources/base.py (Diff revision 9)
     
     
    Show all issues
    Col: 51
     E261 at least two spaces before inline comment
    
  7. djblets/webapi/resources/root.py (Diff revision 9)
     
     
    Show all issues
    Col: 33
     E126 continuation line over-indented for hanging indent
    
  8. djblets/webapi/resources/root.py (Diff revision 9)
     
     
    Show all issues
    Col: 5
     E303 too many blank lines (2)
    
  9. 
      
RS
reviewbot
  1. 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
    
    
  2. 
      
RS
RS
reviewbot
  1. 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
    
    
  2. Show all issues
     'ExtensionResource' imported but unused
    
  3. Show all issues
    Col: 13
     E122 continuation line missing indentation or outdented
    
  4. Show all issues
    Col: 80
     E501 line too long (89 > 79 characters)
    
  5. Show all issues
    Col: 80
     E501 line too long (91 > 79 characters)
    
  6. Show all issues
    Col: 80
     E501 line too long (85 > 79 characters)
    
  7. Show all issues
    Col: 80
     E501 line too long (91 > 79 characters)
    
  8. Show all issues
    Col: 80
     E501 line too long (92 > 79 characters)
    
  9. Show all issues
    Col: 80
     E501 line too long (92 > 79 characters)
    
  10. Show all issues
    Col: 80
     E501 line too long (89 > 79 characters)
    
  11. Show all issues
    Col: 80
     E501 line too long (92 > 79 characters)
    
  12. Show all issues
    Col: 80
     E501 line too long (92 > 79 characters)
    
  13. Show all issues
    Col: 68
     W292 no newline at end of file
    
  14. 
      
brennie
  1. 
      
  2. Show all issues

    test_extensionresourcemixin.py should be test_extension_resource_mixin.py.

  3. djblets/extensions/resources.py (Diff revision 11)
     
     
    Show all issues

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

    1. Eep, stupid markdown.

      This should be:

      :py:class:`djblets.resources.whatever.RootResource`
      
  4. djblets/extensions/resources.py (Diff revision 11)
     
     
    Show all issues

    Missing docstring.

  5. djblets/extensions/resources.py (Diff revision 11)
     
     
    Show all issues

    I don't think this tells us anything.

  6. djblets/webapi/resources/root.py (Diff revision 11)
     
     
    Show all issues

    No blank line here.

  7. 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.

    1. 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).

  8. djblets/webapi/resources/root.py (Diff revision 11)
     
     
     
     
    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.)

  9. djblets/webapi/resources/root.py (Diff revision 11)
     
     
    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.

  10. 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.

  11. djblets/webapi/resources/root.py (Diff revision 11)
     
     
    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).

  12. djblets/webapi/resources/root.py (Diff revision 11)
     
     
    Show all issues

    Missing Args and Yields

    Yields is just like Returns.

  13. djblets/webapi/resources/root.py (Diff revision 11)
     
     
    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.

  14. djblets/webapi/resources/root.py (Diff revision 11)
     
     
     
    Show all issues

    Missing Args.

  15. djblets/webapi/resources/root.py (Diff revision 11)
     
     
     
    Show all issues

    Here, too.

  16. 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."""

    1. That makes a lot of sense.

  17. Show all issues

    Docstring.

  18. djblets/webapi/tests/test_extensionresourcemixin.py (Diff revision 11)
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Missing tests.

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

    2. 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

  19. Show all issues

    Docstring. This should be something like

    """Unit tests for root resource template registration."""
    
  20. 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.

  21. Show all issues

    "URI template cache"

  22. 
      
RS
reviewbot
  1. 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
    
    
  2. Show all issues
    Col: 13
     E122 continuation line missing indentation or outdented
    
  3. Show all issues
    Col: 80
     E501 line too long (89 > 79 characters)
    
  4. Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  5. Show all issues
    Col: 80
     E501 line too long (91 > 79 characters)
    
  6. Show all issues
    Col: 80
     E501 line too long (85 > 79 characters)
    
  7. Show all issues
    Col: 80
     E501 line too long (91 > 79 characters)
    
  8. Show all issues
    Col: 80
     E501 line too long (92 > 79 characters)
    
  9. Show all issues
    Col: 80
     E501 line too long (92 > 79 characters)
    
  10. Show all issues
    Col: 80
     E501 line too long (89 > 79 characters)
    
  11. Show all issues
    Col: 80
     E501 line too long (92 > 79 characters)
    
  12. Show all issues
    Col: 80
     E501 line too long (92 > 79 characters)
    
  13. 
      
brennie
  1. 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.

  2. djblets/extensions/resources.py (Diff revision 12)
     
     
     
     
    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.

  3. djblets/extensions/resources.py (Diff revision 12)
     
     
    Show all issues

    This should line up with the first ".

  4. djblets/extensions/resources.py (Diff revision 12)
     
     
    Show all issues

    Same here.

  5. djblets/extensions/resources.py (Diff revision 12)
     
     
    Show all issues

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

    Missing Args, Returns blocks

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

  6. djblets/extensions/resources.py (Diff revision 12)
     
     
    Show all issues

    And here, too.

  7. djblets/extensions/resources.py (Diff revision 12)
     
     
    Show all issues

    And here.

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

  8. djblets/webapi/resources/root.py (Diff revision 12)
     
     
     
     
     
    Show all issues

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

  9. djblets/webapi/resources/root.py (Diff revision 12)
     
     
     
    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) )

  10. djblets/webapi/resources/root.py (Diff revision 12)
     
     
    Show all issues

    unicode.

  11. djblets/webapi/resources/root.py (Diff revision 12)
     
     
     
     
     
     
     
     
     
     
    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.

    1. 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.

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

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

  13. djblets/webapi/resources/root.py (Diff revision 12)
     
     
     
    Show all issues

    Pull this out, too.

  14. djblets/webapi/resources/root.py (Diff revision 12)
     
     
    Show all issues

    Imperitive mood ("Register")

  15. djblets/webapi/resources/root.py (Diff revision 12)
     
     
    Show all issues

    unicode.

  16. djblets/webapi/resources/root.py (Diff revision 12)
     
     
    Show all issues

    unicode

  17. djblets/webapi/resources/root.py (Diff revision 12)
     
     
    Show all issues

    "Unregister"

    This may not be a word, but what do dictionaries know? They let frobnicate be a word :)

  18. djblets/webapi/resources/root.py (Diff revision 12)
     
     
     
    Show all issues

    unicode

  19. Show all issues

    Not really necessary tbh.

  20. Show all issues

    """Unit tests for the ExtensionRootResourceMixin."""

    No need for full class paths here.

  21. Show all issues

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

  22. 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 :))

  23. Show all issues

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

  24. Show all issues

    No blank line here.

  25. 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.

  26. 
      
RS
RS
brennie
  1. 
      
  2. djblets/extensions/resources.py (Diff revision 14)
     
     
     
    Show all issues

    Blank line between these.

  3. djblets/extensions/resources.py (Diff revision 14)
     
     
    Show all issues

    Missing Args

    1. I'm not actually sure how to document args/kwargs in this case.

  4. djblets/extensions/resources.py (Diff revision 14)
     
     
    Show all issues

    Missing **kwargs (dict)

    Also, what is kwargs used for here?

    1. 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.

  5. djblets/extensions/resources.py (Diff revision 14)
     
     
    Show all issues

    Since this isnt a type object, can we rename this to ext or extension?

  6. djblets/extensions/resources.py (Diff revision 14)
     
     
     
     
     
    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)
    
  7. djblets/extensions/resources.py (Diff revision 14)
     
     
    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?

  8. djblets/extensions/resources.py (Diff revision 14)
     
     
    Show all issues

    Same here re: kwargs.

    1. Same reason above. I've removed it.

  9. djblets/extensions/resources.py (Diff revision 14)
     
     
    Show all issues

    Same here.

  10. djblets/extensions/resources.py (Diff revision 14)
     
     
     
     
     
    Show all issues

    Same thing here.

  11. djblets/extensions/resources.py (Diff revision 14)
     
     
    Show all issues

    Here, too.

  12. djblets/webapi/resources/root.py (Diff revision 14)
     
     
    Show all issues

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

  13. djblets/webapi/resources/root.py (Diff revision 14)
     
     
    Show all issues

    "Attributes".

  14. djblets/webapi/resources/root.py (Diff revision 14)
     
     
     
     
     
     
    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

  15. djblets/webapi/resources/root.py (Diff revision 14)
     
     
    Show all issues

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

  16. djblets/webapi/resources/root.py (Diff revision 14)
     
     
     
    Show all issues

    Blank line between these.

  17. djblets/webapi/resources/root.py (Diff revision 14)
     
     
    Show all issues

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

  18. djblets/webapi/resources/root.py (Diff revision 14)
     
     
    Show all issues

    This should yield self.ResourceEntry(name, list_href, resource, is_list).

  19. djblets/webapi/resources/root.py (Diff revision 14)
     
     
    Show all issues

    And here

  20. djblets/webapi/resources/root.py (Diff revision 14)
     
     
    Show all issues

    And here.

  21. djblets/webapi/resources/root.py (Diff revision 14)
     
     
    Show all issues

    Can we rename this relative_path

  22. djblets/webapi/resources/root.py (Diff revision 14)
     
     
    Show all issues

    Can we name this relative_resource or parent_resource?

  23. djblets/webapi/resources/root.py (Diff revision 14)
     
     
     
    Show all issues

    Blank line between these.

  24. djblets/webapi/resources/root.py (Diff revision 14)
     
     
    Show all issues

    Again, can we rename this relative_resource or parent_resource?

  25. djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 14)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Docstrings for all these

  26. Show all issues

    Can we call this TestChildResourceOne

  27. Show all issues

    TestChildResourceTwo

  28. Show all issues

    TestResourceOne

  29. Show all issues

    TestResourceTwo

  30. Show all issues

    TestExtension

  31. Show all issues

    TestRootResource

  32. Show all issues

    Docstrings should be of the form

    """Single line summary.
    
    Multi-line description.
    """
    
  33. djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 14)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    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/'
                ),
            }
    
  34. Show all issues

    Add a trailing comma.

  35. Show all issues

    Docstrings should be of the form

    """Single line summary.
    
    Multi-line description.
    """
    
  36. 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])
    
  37. Show all issues

    Can we just call these .ext_mgr and .extension?

  38. djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 14)
     
     
     
     
     
    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'},
    }
    
    1. The order shouldn't matter here - but if you'd rather I put None second I can do that.

  39. Show all issues

    Rename as actual_result, expected_result ?

  40. Show all issues

    Reformat this as:

    self.root_resource._uri_templates = {
        'key1': 'value',
        'key2': 'value',
    }
    
  41. Show all issues

    Rename as expected_result, actual_result?

  42. Show all issues

    This should be formatted as:

    self.assertFalse(self.root_resource._registered_uri_tempaltes[
        self.mock_extension])
    
  43. Show all issues

    Reformat this as:

    self.root_resource._uri_templates = {
        'key1': 'value',
        'key2': 'value',
    }
    
  44. Show all issues

    Reformat this as:

    self.root_resource._uri_templates = {
        'key1': 'value',
        'key2': 'value',
    }
    
  45. 
      
brennie
  1. 
      
  2. Show all issues

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

  3. 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"

  4. 
      
reviewbot
  1. 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
    
    
  2. djblets/webapi/resources/root.py (Diff revision 13)
     
     
    Show all issues
    Col: 55
     E201 whitespace after '('
    
  3. djblets/webapi/resources/root.py (Diff revision 13)
     
     
    Show all issues
    Col: 51
     E202 whitespace before ')'
    
  4. Show all issues
    Col: 80
     E501 line too long (87 > 79 characters)
    
  5. Show all issues
    Col: 30
     E126 continuation line over-indented for hanging indent
    
  6. djblets/webapi/resources/root.py (Diff revision 14)
     
     
    Show all issues
    Col: 55
     E201 whitespace after '('
    
  7. djblets/webapi/resources/root.py (Diff revision 14)
     
     
    Show all issues
    Col: 51
     E202 whitespace before ')'
    
  8. Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  9. Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  10. Show all issues
    Col: 30
     E126 continuation line over-indented for hanging indent
    
  11. 
      
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/extensions/resources.py
        djblets/webapi/tests/test_extension_resource_mixin.py
        djblets/webapi/resources/root.py
    
    
  2. Show all issues
     'SpyAgency' imported but unused
    
  3. 
      
RS
RS
RS
reviewbot
  1. 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
    
    
  2. 
      
RS
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. 
      
  2. djblets/extensions/resources.py (Diff revisions 14 - 16)
     
     
     
    Show all issues

    This will fit on one line.

  3. djblets/extensions/resources.py (Diff revisions 14 - 16)
     
     
     
    Show all issues

    Reformat as:

                    self.register_uri_template(entry.name, entry.list_href,
                                               ext_resource)
    
  4. djblets/extensions/resources.py (Diff revisions 14 - 16)
     
     
     
    Show all issues

    This will fit on one line

  5. djblets/extensions/resources.py (Diff revisions 14 - 16)
     
     
     
    Show all issues

    This will fit on one line.

  6. djblets/webapi/resources/root.py (Diff revisions 14 - 16)
     
     
    Show all issues

    djblets.webapi.resources.base.WebAPIResource

  7. djblets/webapi/resources/root.py (Diff revisions 14 - 16)
     
     
    Show all issues

    "Whether or not the resource is a list resource."

  8. djblets/webapi/resources/root.py (Diff revisions 14 - 16)
     
     
     
    Show all issues

    Could you reformat this as:

    templates[registered_name] = (
        '%s%s'
        % (href, registered_href)
    )
    
  9. djblets/webapi/resources/root.py (Diff revisions 14 - 16)
     
     
     
    Show all issues

    Reformat as:

                yield self.ResourceEntry(resource.name, object_href, resource,
                                         False)
    
  10. djblets/webapi/resources/root.py (Diff revisions 14 - 16)
     
     
     
     
     
    Show all issues

    This can be simplified to:

                for entry in self.walk_resources(child, child_href):
                    yield entry
    
  11. djblets/webapi/resources/root.py (Diff revisions 14 - 16)
     
     
     
    Show all issues

    Reformat as:

            self._registered_uri_templates[relative_resource][name] = \
               relative_path
    
  12. 
      
RS
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. This should be the last review pass!

  2. djblets/extensions/resources.py (Diff revision 17)
     
     
    Show all issues

    Can you make a note that the subclass that uses this mixin must implement this?

  3. djblets/webapi/resources/root.py (Diff revision 17)
     
     
     
    Show all issues

    How about:

    for name, href in six.iteritems(unassigned_templates):
        # ....
    
  4. djblets/webapi/resources/root.py (Diff revision 17)
     
     
     
    Show all issues

    for entry in self.walk_resources(self, base_href):
    
  5. djblets/webapi/resources/root.py (Diff revision 17)
     
     
     
     
     
     
     
     
     
     
    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)
                        )
    
  6. djblets/webapi/resources/root.py (Diff revision 17)
     
     
     
     
     
    Show all issues

    This can be simplified to

    for entry in self.walk_resources(child, child_href):
        yield entry
    
  7. djblets/webapi/resources/root.py (Diff revision 17)
     
     
     
     
     
    Show all issues

    for entry in self.walk_resources(child, child_href):
        yield entry 
    
  8. 
      
RS
reviewbot
  1. 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
    
    
  2. 
      
chipx86
  1. 
      
  2. djblets/extensions/resources.py (Diff revision 18)
     
     
    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.

  3. djblets/extensions/resources.py (Diff revision 18)
     
     
     
    Show all issues

    Blank line between these.

  4. djblets/extensions/resources.py (Diff revision 18)
     
     
     
     
     
    Show all issues

    This is missing **kwargs.

  5. djblets/extensions/resources.py (Diff revision 18)
     
     
    Show all issues

    The type needs to be a full class path.

  6. djblets/extensions/resources.py (Diff revision 18)
     
     
     
     
     
    Show all issues

    Same comments as above.

  7. djblets/extensions/resources.py (Diff revision 18)
     
     
     
     
     
    Show all issues

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

  8. djblets/webapi/resources/root.py (Diff revision 18)
     
     
     
    Show all issues

    This should be on the same line.

  9. djblets/webapi/resources/root.py (Diff revision 18)
     
     
     
    Show all issues

    Same line.

  10. djblets/webapi/resources/root.py (Diff revision 18)
     
     
     
     
     
    Show all issues

    This can be one line.

  11. djblets/webapi/resources/root.py (Diff revision 18)
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

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

  12. djblets/webapi/resources/root.py (Diff revision 18)
     
     
    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.

  13. djblets/webapi/resources/root.py (Diff revision 18)
     
     
     
    Show all issues

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

  14. djblets/webapi/resources/root.py (Diff revision 18)
     
     
     
     
    Show all issues

    This should remain one statement.

  15. djblets/webapi/resources/root.py (Diff revision 18)
     
     
     
    Show all issues

    Same comment as above for keyword arguments.

  16. djblets/webapi/resources/root.py (Diff revision 18)
     
     
    Show all issues

    "URI"

  17. djblets/webapi/resources/root.py (Diff revision 18)
     
     
    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.

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

    This should say what it's relative to.

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

  19. djblets/webapi/resources/root.py (Diff revision 18)
     
     
    Show all issues

    Needs to be an absolute class path.

    Should also include , optional after the type.

  20. djblets/webapi/resources/root.py (Diff revision 18)
     
     
     
     
     
     
     
     
     
    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
    
  21. djblets/webapi/resources/root.py (Diff revision 18)
     
     
     
     
     
     
    Show all issues

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

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

  22. djblets/webapi/resources/root.py (Diff revision 18)
     
     
    Show all issues

    "URI"

  23. djblets/webapi/resources/root.py (Diff revision 18)
     
     
     
     
    Show all issues

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

  24. djblets/webapi/resources/root.py (Diff revision 18)
     
     
     
    Show all issues

    Full class path and , optional.

  25. djblets/webapi/resources/root.py (Diff revision 18)
     
     
    Show all issues

    "URI"

  26. djblets/webapi/resources/root.py (Diff revision 18)
     
     
     
    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).

    1. Ah, this is a much better solution.

  27. Show all issues

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

    1. Whoops, looks like I removed a line by accident.

  28. Show all issues

    These can be one import statement.

  29. Show all issues

    Please use numbers instead of spelling them out for these classes.

  30. Show all issues

    Missing a trailing period.

    Same with the ones below.

  31. 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.

    1. 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.

  32. Show all issues

    This should probably be ExtensionRootResourceMixinTests.

  33. Show all issues

    This should be done first, probably.

    Blank line between it and the setters.

    1. 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.

  34. 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.

  35. Show all issues

    This whole thing should be used inline below, like:

    self.assertEqual(
        actual_result,
        {
            ...
        })
    
  36. 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.

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

  37. djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 18)
     
     
     
     
     
     
    Show all issues

    This should also be inline below.

    Same with other tests.

  38. Show all issues

    This should be RootResourceTests, and should live in its own file (test_root_resource.py).

  39. djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 18)
     
     
     
     
     
    Show all issues

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

    .... = {
        self.ext_resource: {
            'extensions': ...,
        },
        None: {
            'extensions': ...,
        },
    }
    
  40. 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.

  41. Show all issues

    This can be one line.

  42. Show all issues

    These calls will read better if you use keyword arguments.

  43. Show all issues

    Should be inline.

  44. Show all issues

    Condense this.

  45. 
      
RS
reviewbot
  1. 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
    
    
  2. djblets/webapi/resources/root.py (Diff revision 19)
     
     
    Show all issues
    Col: 50
     E225 missing whitespace around operator
    
  3. Show all issues
    Col: 9
     E303 too many blank lines (2)
    
  4. 
      
RS
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. 
      
  2. Show all issues

    "URI" in your summary.

  3. djblets/extensions/resources.py (Diff revision 20)
     
     
     
    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.

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

  4. djblets/extensions/resources.py (Diff revision 20)
     
     
    Show all issues

    "the URI templates."

  5. djblets/extensions/resources.py (Diff revision 20)
     
     
     
    Show all issues

    Same here.

  6. djblets/extensions/resources.py (Diff revision 20)
     
     
    Show all issues

    "the URI templates."

  7. djblets/webapi/resources/root.py (Diff revision 20)
     
     
     
    Show all issues

    Blank line between these.

  8. djblets/webapi/resources/root.py (Diff revision 20)
     
     
    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 ?

  9. djblets/webapi/resources/root.py (Diff revision 20)
     
     
    Show all issues

    Please elaborate on this.

  10. djblets/webapi/resources/root.py (Diff revision 20)
     
     
     
    Show all issues

    Don't wrap this. You will get a error from review bot but you can drop it.

  11. djblets/webapi/tests/test_root_resource.py (Diff revision 20)
     
     
     
    Show all issues

    Blank line between these

  12. 
      
RS
reviewbot
  1. 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
    
    
  2. djblets/webapi/resources/root.py (Diff revision 21)
     
     
    Show all issues
    Col: 80
     E501 line too long (89 > 79 characters)
    
  3. djblets/webapi/resources/root.py (Diff revision 21)
     
     
    Show all issues
    Col: 80
     E501 line too long (89 > 79 characters)
    
  4. 
      
mike_conley
  1. 
      
  2. Show all issues

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

  3. djblets/extensions/resources.py (Diff revision 21)
     
     
     
     
     
     
     
    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.
    1. The documentation request came from another mentor, so I'll keep it as-is for now if that's alright.

  4. djblets/webapi/resources/root.py (Diff revision 21)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    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.

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

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

  5. djblets/webapi/resources/root.py (Diff revision 21)
     
     
    Show all issues

    Leftovers?

  6. 
      
RS
RS
reviewbot
  1. 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
    
    
  2. djblets/webapi/resources/root.py (Diff revision 22)
     
     
    Show all issues
    Col: 80
     E501 line too long (89 > 79 characters)
    
  3. djblets/webapi/resources/root.py (Diff revision 22)
     
     
    Show all issues
    Col: 80
     E501 line too long (89 > 79 characters)
    
  4. 
      
chipx86
  1. Almost at the finish line! Everything here is either a tiny doc fix or a tiny syntax/consistency issue.

  2. Show all issues

    test_root_resource.py is missing a module docstring.

  3. djblets/extensions/resources.py (Diff revision 22)
     
     
    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.

  4. djblets/extensions/resources.py (Diff revision 22)
     
     
    Show all issues

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

  5. djblets/extensions/resources.py (Diff revision 22)
     
     
    Show all issues

    No parens.

  6. djblets/webapi/resources/root.py (Diff revision 22)
     
     
    Show all issues

    No outer parens needed.

  7. Show all issues

    No parens.

  8. 
      
RS
reviewbot
  1. 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
    
    
  2. djblets/webapi/resources/root.py (Diff revision 23)
     
     
    Show all issues
    Col: 80
     E501 line too long (89 > 79 characters)
    
  3. djblets/webapi/resources/root.py (Diff revision 23)
     
     
    Show all issues
    Col: 80
     E501 line too long (89 > 79 characters)
    
  4. 
      
chipx86
  1. This looks great! I just have a couple minor things left, and then I think we're ready to land this :)

  2. djblets/webapi/resources/root.py (Diff revision 23)
     
     
     
     
    Show all issues

    This is indented one level too far.

  3. djblets/webapi/resources/root.py (Diff revision 23)
     
     
    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.

  4. djblets/webapi/resources/root.py (Diff revision 23)
     
     
    Show all issues

    Same comment here as above with a comment.

  5. 
      
RS
reviewbot
  1. 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
    
    
  2. djblets/webapi/resources/root.py (Diff revision 24)
     
     
    Show all issues
    Col: 80
     E501 line too long (89 > 79 characters)
    
  3. djblets/webapi/resources/root.py (Diff revision 24)
     
     
    Show all issues
    Col: 80
     E501 line too long (89 > 79 characters)
    
  4. 
      
FI
  1. 
      
  2. 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).

    1. 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.

  3. 
      
brennie
  1. Two TINY nits. This looks good to me!

  2. djblets/extensions/resources.py (Diff revision 24)
     
     
    Show all issues

    This doesn't need to be a raw string.

  3. djblets/extensions/resources.py (Diff revision 24)
     
     
    Show all issues

    Again, this doesn't need to be a raw string.

  4. 
      
RS
reviewbot
  1. 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
    
    
  2. djblets/webapi/resources/root.py (Diff revision 25)
     
     
    Show all issues
    Col: 80
     E501 line too long (89 > 79 characters)
    
  3. djblets/webapi/resources/root.py (Diff revision 25)
     
     
    Show all issues
    Col: 80
     E501 line too long (89 > 79 characters)
    
  4. 
      
chipx86
  1. 
      
  2. djblets/webapi/resources/root.py (Diff revisions 23 - 25)
     
     
    Show all issues

    Missing trailing period. All comments should be in full sentence format.

  3. djblets/webapi/resources/root.py (Diff revisions 23 - 25)
     
     
    Show all issues

    Same here.

  4. 
      
mike_conley
MA
Review request changed
Change Summary:

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

Branch:
release-0.10.x
master
Commit:
c4ef69eb101cab23f8ea5ff8bb8e24694d25c1f8
5e639a88dadbef8d33f8d5a453d787fb5d353a16

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

MA
david
  1. Ship It!
  2. 
      
MA
Review request changed
Status:
Completed
Change Summary:
Pushed to release-2.0.x (d84cc41)