Allow Web API Capabilities Integration through ReviewBoard Extensions

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

justy777
Review Board
master
6641
b112f63...
reviewboard, students

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)
     
     
     
     
     

    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)
     
     

    Only one underscore.

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

    No blank line.

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

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

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

    should be all lowercase. "webapi"

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

    No blank line.

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

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

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

    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)
     
     
     
     

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

    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)
     
     

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

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

    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)
     
     

    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)
     
     

    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)
     
     

    """ on the next line.

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

    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)
     
     

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

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

    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. Col: 22
     E201 whitespace after '('
    
  3. Col: 22
     E128 continuation line under-indented for visual indent
    
  4. 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. 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.

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

  4. That alignment violates PEP-8. Instead, do:

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

    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.

  6. reviewboard/extensions/hooks.py (Diff revision 9)
     
     
     
     

    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.

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

    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)
     
     

    End with a period.

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

    End with a period.

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

  2. Missing space before "The".

  3. docs/manual/extending/extensions/hooks/webapi-capabilities-hook.rst (Diff revision 10)
     
     
     
     
     
     
     
     
     
     
     
     

    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
       }
    }
    
  4. reviewboard/extensions/hooks.py (Diff revision 10)
     
     
     

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

  5. reviewboard/extensions/tests.py (Diff revision 10)
     
     
     
     
     
     
     
     
     
     

    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.

  6. 
      
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: Closed (submitted)

Change Summary:

Pushed to master (40e705f)
Loading...