Allow Web API Capabilities Integration through ReviewBoard Extensions

Review Request #6613 — Created Nov. 21, 2014 and submitted

Information

Review Board
master
b112f63...

Reviewers

There's now a WebAPICapabilitiesHook that extensions can use to register a set of web API capabilites. This handles registering, and unregistering the sets of web API capabilities.
Also Provides documentation and sample code for the newly created WebAPICapabilitiesHook.

Unit test have been written to make sure that Capabilities are registered and unregistered properly. The third unit test checks that None cannot be registered as a key. The fourth unit test also checks that none of the default capabilities can be changed or removed.

Description From Last Updated

The dictionary is awkwardly formatted. Something like this would be better: WebAPICapabilityHook( extension=self.extension, capabilities_id='tests', caps={ 'me': True, 'him': True, }) …

chipx86chipx86

Only one underscore.

chipx86chipx86

No blank line.

chipx86chipx86

This will override _capabilities_defaults every time. We instead need to make a copy.

chipx86chipx86

should be all lowercase. "webapi"

chipx86chipx86

No blank line.

chipx86chipx86

This should be a ValueError, since it's the value that's wrong.

chipx86chipx86

This will fail if capabilities_id is not in _registered_capabilities. Instead, this should be: if capabilities_id in _registred_capabilities: Same below.

chipx86chipx86

No blank line.

chipx86chipx86

That first part doesn't make any sense. We're not subclassing attribute. We actually don't subclass at all. Along with these …

chipx86chipx86

The example should just show this with the dictionary as part of the hook call, rather than as a global …

chipx86chipx86

We should call this WebAPICapabilitiesHook, since it handles more than one capability at a time.

chipx86chipx86

I don't think we should allow the capabilities ID to be set by the caller. I think we should use …

chipx86chipx86

No need to do this if we're setting it below. Actually, we should set the capabilities ID here, and then …

chipx86chipx86

We should never allow this to be the case. We should raise an exception in __init__ if the capabilities ID …

chipx86chipx86

""" on the next line.

chipx86chipx86

We don't need to deepcopy this, since there's no risk of children of any of these keys being modified as …

chipx86chipx86

How about "is reserved for the default set of capabilities" ?

chipx86chipx86

This will be faster as: try: del .... except KeyError: logging.... raise ...

chipx86chipx86

Col: 22 E201 whitespace after '('

reviewbotreviewbot

Col: 22 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 25 E201 whitespace after '('

reviewbotreviewbot

WebAPICapabilitiesHook

chipx86chipx86

This is a bit difficult to read. How about: "The API capabilities payload will contain a new key matching the …

chipx86chipx86

This isn't a class, and it's an internal detail that we don't want to expose in the documentation.

chipx86chipx86

That alignment violates PEP-8. Instead, do: WebAPICapabilitiesHook( self, { 'commit_ids': True, 'tested': True, })

chipx86chipx86

This is confusing and doesn't explain itself well, especially since it appears as its own section, without any code formatting. …

chipx86chipx86

Shutting down should only ever be done by the extension manager. We shouldn't be sandboxing RB code. We'll just cover …

chipx86chipx86

Missing space before "The".

chipx86chipx86

"payload"

chipx86chipx86

A couple things here: In JSON payloads, double quotes are required. Not really work showing all the native keys. How …

chipx86chipx86

It never will be in practice, so no need to check this.

chipx86chipx86

End with a period.

ML mloyzer

End with a period.

ML mloyzer

I think you'll get a pep8 error here. If not, you're lucky. Certainly the capabilities dictionary isn't indented 4 spaces, …

chipx86chipx86

End with a period.

ML mloyzer
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/extensions/hooks.py
        reviewboard/extensions/tests.py
        reviewboard/webapi/server_info.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/extensions/hooks.py
        reviewboard/extensions/tests.py
        reviewboard/webapi/server_info.py
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/extensions/tests.py (Diff revision 1)
     
     
     
     
     
    Show all issues

    The dictionary is awkwardly formatted. Something like this would be better:

    WebAPICapabilityHook(
        extension=self.extension,
        capabilities_id='tests',
        caps={
            'me': True,
            'him': True,
        })
    

    Same below.

  3. reviewboard/webapi/server_info.py (Diff revision 1)
     
     
    Show all issues

    Only one underscore.

  4. reviewboard/webapi/server_info.py (Diff revision 1)
     
     
     
     
    Show all issues

    No blank line.

  5. reviewboard/webapi/server_info.py (Diff revision 1)
     
     
     
    Show all issues

    This will override _capabilities_defaults every time. We instead need to make a copy.

  6. reviewboard/webapi/server_info.py (Diff revision 1)
     
     
    Show all issues

    should be all lowercase. "webapi"

  7. reviewboard/webapi/server_info.py (Diff revision 1)
     
     
     
     
    Show all issues

    No blank line.

  8. reviewboard/webapi/server_info.py (Diff revision 1)
     
     
    Show all issues

    This should be a ValueError, since it's the value that's wrong.

  9. reviewboard/webapi/server_info.py (Diff revision 1)
     
     
    Show all issues

    This will fail if capabilities_id is not in _registered_capabilities. Instead, this should be:

    if capabilities_id in _registred_capabilities:
    

    Same below.

  10. reviewboard/webapi/server_info.py (Diff revision 1)
     
     
     
     
    Show all issues

    No blank line.

  11. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/extensions/hooks.py
        reviewboard/extensions/tests.py
        reviewboard/webapi/server_info.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/extensions/hooks.py
        reviewboard/extensions/tests.py
        reviewboard/webapi/server_info.py
    
    
  2. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/extensions/hooks.py
        reviewboard/extensions/tests.py
        reviewboard/webapi/server_info.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/extensions/hooks.py
        reviewboard/extensions/tests.py
        reviewboard/webapi/server_info.py
    
    
  2. 
      
justy777
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/extensions/hooks.py
        reviewboard/extensions/tests.py
        reviewboard/webapi/server_info.py
    
    Ignored Files:
        docs/manual/extending/extensions/hooks/index.rst
        docs/manual/extending/extensions/hooks/webapi-capability-hook.rst
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/extensions/hooks.py
        reviewboard/extensions/tests.py
        reviewboard/webapi/server_info.py
    
    Ignored Files:
        docs/manual/extending/extensions/hooks/index.rst
        docs/manual/extending/extensions/hooks/webapi-capability-hook.rst
    
    
  2. 
      
justy777
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/extensions/hooks.py
        reviewboard/extensions/tests.py
        reviewboard/webapi/server_info.py
    
    Ignored Files:
        docs/manual/extending/extensions/hooks/index.rst
        docs/manual/extending/extensions/hooks/webapi-capability-hook.rst
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/extensions/hooks.py
        reviewboard/extensions/tests.py
        reviewboard/webapi/server_info.py
    
    Ignored Files:
        docs/manual/extending/extensions/hooks/index.rst
        docs/manual/extending/extensions/hooks/webapi-capability-hook.rst
    
    
  2. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/extensions/hooks.py
        reviewboard/extensions/tests.py
        reviewboard/webapi/server_info.py
    
    Ignored Files:
        docs/manual/extending/extensions/hooks/index.rst
        docs/manual/extending/extensions/hooks/webapi-capability-hook.rst
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/extensions/hooks.py
        reviewboard/extensions/tests.py
        reviewboard/webapi/server_info.py
    
    Ignored Files:
        docs/manual/extending/extensions/hooks/index.rst
        docs/manual/extending/extensions/hooks/webapi-capability-hook.rst
    
    
  2. 
      
chipx86
  1. 
      
  2. Show all issues

    That first part doesn't make any sense. We're not subclassing attribute. We actually don't subclass at all.

    Along with these docs, can you show how the resulting API payload would look, given a sample capabilities payload?

  3. docs/manual/extending/extensions/hooks/webapi-capability-hook.rst (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    The example should just show this with the dictionary as part of the hook call, rather than as a global variable.

  4. reviewboard/extensions/hooks.py (Diff revision 6)
     
     
    Show all issues

    We should call this WebAPICapabilitiesHook, since it handles more than one capability at a time.

  5. reviewboard/extensions/hooks.py (Diff revision 6)
     
     
    Show all issues

    I don't think we should allow the capabilities ID to be set by the caller. I think we should use the extension ID. That ensures we have no duplicates, that it's predictable, and that it cannot clash with any future capabilities we add.

  6. reviewboard/extensions/hooks.py (Diff revision 6)
     
     
    Show all issues

    No need to do this if we're setting it below.

    Actually, we should set the capabilities ID here, and then register.

  7. reviewboard/extensions/hooks.py (Diff revision 6)
     
     
    Show all issues

    We should never allow this to be the case. We should raise an exception in __init__ if the capabilities ID is None.

  8. reviewboard/extensions/tests.py (Diff revision 6)
     
     
    Show all issues

    """ on the next line.

  9. reviewboard/webapi/server_info.py (Diff revision 6)
     
     
    Show all issues

    We don't need to deepcopy this, since there's no risk of children of any of these keys being modified as part of this code. A shallow copy is faster and is all we need. For that, just use _capabilities_defaults.copy().

  10. reviewboard/webapi/server_info.py (Diff revision 6)
     
     
    Show all issues

    How about "is reserved for the default set of capabilities" ?

  11. reviewboard/webapi/server_info.py (Diff revision 6)
     
     
     
     
     
     
     
     
     
    Show all issues

    This will be faster as:

    try:
        del ....
    except KeyError:
        logging....
        raise ...
    
  12. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/extensions/tests.py
        reviewboard/webapi/server_info.py
        reviewboard/extensions/hooks.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/webapi/resources/validate_diff.py
        reviewboard/webapi/tests/mixins.py
    
    Ignored Files:
        docs/manual/extending/extensions/hooks/index.rst
        docs/manual/extending/extensions/hooks/webapi-capabilities-hook.rst
        docs/manual/extending/extensions/hooks/webapi-capability-hook.rst
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/extensions/tests.py
        reviewboard/webapi/server_info.py
        reviewboard/extensions/hooks.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/webapi/resources/validate_diff.py
        reviewboard/webapi/tests/mixins.py
    
    Ignored Files:
        docs/manual/extending/extensions/hooks/index.rst
        docs/manual/extending/extensions/hooks/webapi-capabilities-hook.rst
        docs/manual/extending/extensions/hooks/webapi-capability-hook.rst
    
    
  2. Show all issues
    Col: 22
     E201 whitespace after '('
    
  3. Show all issues
    Col: 22
     E128 continuation line under-indented for visual indent
    
  4. Show all issues
    Col: 25
     E201 whitespace after '('
    
  5. 
      
justy777
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/extensions/hooks.py
        reviewboard/extensions/tests.py
        reviewboard/webapi/server_info.py
    
    Ignored Files:
        docs/manual/extending/extensions/hooks/index.rst
        docs/manual/extending/extensions/hooks/webapi-capabilities-hook.rst
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/extensions/hooks.py
        reviewboard/extensions/tests.py
        reviewboard/webapi/server_info.py
    
    Ignored Files:
        docs/manual/extending/extensions/hooks/index.rst
        docs/manual/extending/extensions/hooks/webapi-capabilities-hook.rst
    
    
  2. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/extensions/hooks.py
        reviewboard/extensions/tests.py
        reviewboard/webapi/server_info.py
    
    Ignored Files:
        docs/manual/extending/extensions/hooks/index.rst
        docs/manual/extending/extensions/hooks/webapi-capabilities-hook.rst
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/extensions/hooks.py
        reviewboard/extensions/tests.py
        reviewboard/webapi/server_info.py
    
    Ignored Files:
        docs/manual/extending/extensions/hooks/index.rst
        docs/manual/extending/extensions/hooks/webapi-capabilities-hook.rst
    
    
  2. 
      
chipx86
  1. 
      
  2. Show all issues

    WebAPICapabilitiesHook

  3. Show all issues

    This is a bit difficult to read. How about:

    "The API capabilities payload will contain a new key matching the extension ID, with the provided capabilities as the value."

    Also, this line should start on the previous line, wrapped correctly, since it's part of the same paragraph.

    Trailing whitespace should be removed.

  4. Show all issues

    This isn't a class, and it's an internal detail that we don't want to expose in the documentation.

  5. Show all issues

    That alignment violates PEP-8. Instead, do:

    WebAPICapabilitiesHook(
        self,
        {
            'commit_ids': True,
            'tested': True,
        })
    
  6. docs/manual/extending/extensions/hooks/webapi-capabilities-hook.rst (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    This is confusing and doesn't explain itself well, especially since it appears as its own section, without any code formatting.

    I'd merge this into the Example section, with some text saying that the resulting payload would look like "<payload>". You'd want to use .. code-block:: javascript above it.

  7. reviewboard/extensions/hooks.py (Diff revision 9)
     
     
     
     
    Show all issues

    Shutting down should only ever be done by the extension manager.

    We shouldn't be sandboxing RB code. We'll just cover up errors that way.

  8. 
      
justy777
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/extensions/hooks.py
        reviewboard/extensions/tests.py
        reviewboard/webapi/server_info.py
    
    Ignored Files:
        docs/manual/extending/extensions/hooks/index.rst
        docs/manual/extending/extensions/hooks/webapi-capabilities-hook.rst
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/extensions/hooks.py
        reviewboard/extensions/tests.py
        reviewboard/webapi/server_info.py
    
    Ignored Files:
        docs/manual/extending/extensions/hooks/index.rst
        docs/manual/extending/extensions/hooks/webapi-capabilities-hook.rst
    
    
  2. 
      
ML
  1. Small things:

  2. reviewboard/extensions/tests.py (Diff revision 10)
     
     
    Show all issues

    End with a period.

    1. Docstrings for tests in Review Board do not end in a period.

  3. reviewboard/extensions/tests.py (Diff revision 10)
     
     
    Show all issues

    End with a period.

  4. reviewboard/extensions/tests.py (Diff revision 10)
     
     
    Show all issues

    End with a period.

  5. 
      
chipx86
  1. Almost done! Couple small things.

  2. Show all issues

    Missing space before "The".

  3. Show all issues

    "payload"

  4. docs/manual/extending/extensions/hooks/webapi-capabilities-hook.rst (Diff revision 10)
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    A couple things here:

    • In JSON payloads, double quotes are required.
    • Not really work showing all the native keys. How about just replacing them with "..."
    • For the SampleExtensionID one, can you pretty-print it?

    So:

    "capabilities": {
       ...
    
       "SampleExtensionID": {
           "commit_ids": True,
           "tested": True
       }
    }
    
  5. reviewboard/extensions/hooks.py (Diff revision 10)
     
     
     
    Show all issues

    It never will be in practice, so no need to check this.

  6. reviewboard/extensions/tests.py (Diff revision 10)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    I think you'll get a pep8 error here. If not, you're lucky. Certainly the capabilities dictionary isn't indented 4 spaces, which may be satisfying the tool, but not the coding style.

    So instead:

    self.assertRaisesMessage(
        KeyError,
        '...',
        WebAPICapabilitiesHook,
        self.extension,
        {
            ...
        })
    

    Same in other cases.

  7. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/extensions/hooks.py
        reviewboard/extensions/tests.py
        reviewboard/webapi/server_info.py
    
    Ignored Files:
        docs/manual/extending/extensions/hooks/index.rst
        docs/manual/extending/extensions/hooks/webapi-capabilities-hook.rst
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/extensions/hooks.py
        reviewboard/extensions/tests.py
        reviewboard/webapi/server_info.py
    
    Ignored Files:
        docs/manual/extending/extensions/hooks/index.rst
        docs/manual/extending/extensions/hooks/webapi-capabilities-hook.rst
    
    
  2. 
      
justy777
Review request changed
Status:
Completed
Change Summary:
Pushed to master (40e705f)