Providing URI templates for extensions

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

maram
Djblets
master
3ef58d3...
djblets, students
chipx86

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.

  • 0
  • 0
  • 259
  • 21
  • 280
Description From Last Updated
  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. Col: 1
     E302 expected 2 blank lines, found 1
    
  3. Col: 80
     E501 line too long (82 > 79 characters)
    
  4. Col: 1
     W391 blank line at end of file
    
  5. djblets/webapi/resources/root.py (Diff revision 1)
     
     
    Col: 69
     E261 at least two spaces before inline comment
    
  6. djblets/webapi/resources/root.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (88 > 79 characters)
    
  7. djblets/webapi/resources/root.py (Diff revision 1)
     
     
    Col: 59
     W292 no newline at end of file
    
  8. 
      
  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. Col: 19
     W292 no newline at end of file
    
  3. djblets/webapi/resources/root.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  4. djblets/webapi/resources/root.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (82 > 79 characters)
    
  5. 
      
  1. 
      
  2. djblets/webapi/resources/root.py (Diff revision 2)
     
     

    This will save you a lot of review changes.

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

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

    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)
     
     

    Missing a period.

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

    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)
     
     
     

    The comment should be a docstring.

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

    Missing docstring.

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

    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)
     
     
     

    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)
     
     
     

    This should be at the top of the file.

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

    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)
     
     

    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)
     
     
     

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

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

    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)
     
     
     
     
     
     

    Should be 2 blank lines.

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

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

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

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

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

    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)
     
     

    Missing docstring.

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

    I don't think this tells us anything.

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

    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)
     
     
     
     

    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)
     
     

    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)
     
     

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

    Missing Args and Yields

    Yields is just like Returns.

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

    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)
     
     
     

    Missing Args.

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

    Here, too.

  16. 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. djblets/webapi/tests/test_extensionresourcemixin.py (Diff revision 11)
     
     
     
     
     
     
     
     
     
     
     
     

    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

  18. Docstring. This should be something like

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

  20. "URI template cache"

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

    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)
     
     

    This should line up with the first ".

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

    Same here.

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

    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)
     
     

    And here, too.

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

    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)
     
     
     
     
     

    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)
     
     
     

    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)
     
     

    unicode.

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

    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)
     
     
     

    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)
     
     
     

    Pull this out, too.

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

    Imperitive mood ("Register")

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

    unicode.

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

    unicode

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

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

    unicode

  19. Not really necessary tbh.

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

    No need for full class paths here.

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

  22. 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. setUp must call super(RootResourceTemplateRegistrationTests, self).setUp() or you're going to end up with weird, unsolvable test failures sometimes :)

  24. No blank line here.

  25. 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. 
      
brennie
  1. 
      
  2. djblets/extensions/resources.py (Diff revision 14)
     
     
     

    Blank line between these.

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

    Missing Args

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

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

    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)
     
     

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

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

    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)
     
     

    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)
     
     

    Same here re: kwargs.

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

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

    Same here.

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

    Same thing here.

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

    Here, too.

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

    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)
     
     

    "Attributes".

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

    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)
     
     

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

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

    Blank line between these.

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

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

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

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

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

    And here

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

    And here.

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

    Can we rename this relative_path

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

    Can we name this relative_resource or parent_resource?

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

    Blank line between these.

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

    Again, can we rename this relative_resource or parent_resource?

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

    Docstrings for all these

  26. Can we call this TestChildResourceOne

  27. TestChildResourceTwo

  28. TestResourceOne

  29. TestResourceTwo

  30. Docstrings should be of the form

    """Single line summary.
    
    Multi-line description.
    """
    
  31. djblets/webapi/tests/test_extension_resource_mixin.py (Diff revision 14)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    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/'
                ),
            }
    
  32. Add a trailing comma.

  33. Docstrings should be of the form

    """Single line summary.
    
    Multi-line description.
    """
    
  34. 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])
    
  35. Can we just call these .ext_mgr and .extension?

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

    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.

  37. Rename as actual_result, expected_result ?

  38. Reformat this as:

    self.root_resource._uri_templates = {
        'key1': 'value',
        'key2': 'value',
    }
    
  39. Rename as expected_result, actual_result?

  40. This should be formatted as:

    self.assertFalse(self.root_resource._registered_uri_tempaltes[
        self.mock_extension])
    
  41. Reformat this as:

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

    self.root_resource._uri_templates = {
        'key1': 'value',
        'key2': 'value',
    }
    
  43. 
      
brennie
  1. 
      
  2. "Presently The" in your description should be "Presently the"

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

    This will fit on one line.

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

    Reformat as:

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

    This will fit on one line

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

    This will fit on one line.

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

    djblets.webapi.resources.base.WebAPIResource

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

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

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

    Could you reformat this as:

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

    Reformat as:

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

    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)
     
     
     

    Reformat as:

            self._registered_uri_templates[relative_resource][name] = \
               relative_path
    
  12. 
      
  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)
     
     

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

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

    How about:

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

    for entry in self.walk_resources(self, base_href):
    
  5. djblets/webapi/resources/root.py (Diff revision 17)
     
     
     
     
     
     
     
     
     
     

    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)
     
     
     
     
     

    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)
     
     
     
     
     

    for entry in self.walk_resources(child, child_href):
        yield entry 
    
  8. 
      
  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)
     
     

    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)
     
     
     

    Blank line between these.

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

    This is missing **kwargs.

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

    The type needs to be a full class path.

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

    Same comments as above.

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

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

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

    This should be on the same line.

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

    Same line.

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

    This can be one line.

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

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

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

    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)
     
     
     

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

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

    This should remain one statement.

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

    Same comment as above for keyword arguments.

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

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

    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)
     
     

    Needs to be an absolute class path.

    Should also include , optional after the type.

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

    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)
     
     
     
     
     
     

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

    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)
     
     
     

    Full class path and , optional.

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

    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. 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. These can be one import statement.

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

  30. Missing a trailing period.

    Same with the ones below.

  31. 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. This should probably be ExtensionRootResourceMixinTests.

  33. 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. 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. This whole thing should be used inline below, like:

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

    This should also be inline below.

    Same with other tests.

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

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

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

  42. These calls will read better if you use keyword arguments.

  43. Should be inline.

  44. Condense this.

  45. 
      
  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)
     
     
    Col: 50
     E225 missing whitespace around operator
    
  3. Col: 9
     E303 too many blank lines (2)
    
  4. 
      
  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. "URI" in your summary.

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

    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)
     
     

    "the URI templates."

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

    Same here.

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

    "the URI templates."

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

    Blank line between these.

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

    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)
     
     

    Please elaborate on this.

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

    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)
     
     
     

    Blank line between these

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