Allow Web API Capabilities Integration through ReviewBoard Extensions
Review Request #6613 — Created Nov. 21, 2014 and submitted
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, }) … |
 chipx86
|
|
|
Only one underscore. |
chipx86
|
|
|
No blank line. |
chipx86
|
|
|
This will override _capabilities_defaults every time. We instead need to make a copy. |
chipx86
|
|
|
should be all lowercase. "webapi" |
chipx86
|
|
|
No blank line. |
chipx86
|
|
|
This should be a ValueError, since it's the value that's wrong. |
chipx86
|
|
|
This will fail if capabilities_id is not in _registered_capabilities. Instead, this should be: if capabilities_id in _registred_capabilities: Same below. |
chipx86
|
|
|
No blank line. |
chipx86
|
|
|
That first part doesn't make any sense. We're not subclassing attribute. We actually don't subclass at all. Along with these … |
chipx86
|
|
|
The example should just show this with the dictionary as part of the hook call, rather than as a global … |
chipx86
|
|
|
We should call this WebAPICapabilitiesHook, since it handles more than one capability at a time. |
chipx86
|
|
|
I don't think we should allow the capabilities ID to be set by the caller. I think we should use … |
chipx86
|
|
|
No need to do this if we're setting it below. Actually, we should set the capabilities ID here, and then … |
chipx86
|
|
|
We should never allow this to be the case. We should raise an exception in __init__ if the capabilities ID … |
chipx86
|
|
|
""" on the next line. |
chipx86
|
|
|
We don't need to deepcopy this, since there's no risk of children of any of these keys being modified as … |
chipx86
|
|
|
How about "is reserved for the default set of capabilities" ? |
chipx86
|
|
|
This will be faster as: try: del .... except KeyError: logging.... raise ... |
chipx86
|
|
|
Col: 22 E201 whitespace after '(' |
 reviewbot
|
|
|
Col: 22 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 25 E201 whitespace after '(' |
reviewbot
|
|
|
WebAPICapabilitiesHook |
chipx86
|
|
|
This is a bit difficult to read. How about: "The API capabilities payload will contain a new key matching the … |
chipx86
|
|
|
This isn't a class, and it's an internal detail that we don't want to expose in the documentation. |
chipx86
|
|
|
That alignment violates PEP-8. Instead, do: WebAPICapabilitiesHook( self, { 'commit_ids': True, 'tested': True, }) |
chipx86
|
|
|
This is confusing and doesn't explain itself well, especially since it appears as its own section, without any code formatting. … |
chipx86
|
|
|
Shutting down should only ever be done by the extension manager. We shouldn't be sandboxing RB code. We'll just cover … |
chipx86
|
|
|
Missing space before "The". |
chipx86
|
|
|
"payload" |
chipx86
|
|
|
A couple things here: In JSON payloads, double quotes are required. Not really work showing all the native keys. How … |
chipx86
|
|
|
It never will be in practice, so no need to check this. |
chipx86
|
|
|
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, … |
chipx86
|
|
|
End with a period. |
ML mloyzer |
-
-
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.
-
-
-
-
-
-
-
This will fail if
capabilities_idis not in_registered_capabilities. Instead, this should be:if capabilities_id in _registred_capabilities:Same below.
-
-
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
-
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
- Description:
-
There's now a WebAPICapabilityHook that extensions can use to register a set of web API capabilites. This handles registering, and unregistering the sets of web API capabilities.
TODO:
~ Finish writting the api_get portion of the tests ~ Finish writting the api_get portion of the tests - Writting more descriptive comments
- Description:
-
~ There's now a WebAPICapabilityHook that extensions can use to register a set of web API capabilites. This handles registering, and unregistering the sets of web API capabilities.
~ There's now a WebAPICapabilityHook 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 WebAPICapabilityHook. TODO:
Finish writting the api_get portion of the tests - Commit:
-
73246b48b5b636aeabc8d044c461633d850b48192cdbe6071eaed3ad3b6b355a5259d045891773a1
-
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
- Summary:
-
[WIP] Allow Web API Capability Integration through ReviewBoard ExtensionsAllow Web API Capability Integration through ReviewBoard Extensions
- Description:
-
There's now a WebAPICapabilityHook 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 WebAPICapabilityHook. - - TODO:
- Finish writting the api_get portion of the tests - Testing Done:
-
+ Unit test have been written to make sure that Capabilities are registered and unregistered properly. The third unit test also checks that none of the default capabilities can be changed or removed.
- Commit:
-
2cdbe6071eaed3ad3b6b355a5259d045891773a19a29cc206a00dc09cdff9523552e210448077208
-
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
-
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
-
-
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?
-
The example should just show this with the dictionary as part of the hook call, rather than as a global variable.
-
-
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.
-
No need to do this if we're setting it below.
Actually, we should set the capabilities ID here, and then register.
-
We should never allow this to be the case. We should raise an exception in
__init__if the capabilities ID isNone. -
-
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(). -
-
- Summary:
-
Allow Web API Capability Integration through ReviewBoard ExtensionsAllow Web API Capabilities Integration through ReviewBoard Extensions
- Description:
-
~ There's now a WebAPICapabilityHook 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 WebAPICapabilityHook. ~ 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. - Commit:
-
ee530e724a1f25f617fd556de76d52457a9dba73f4e27f7fd624fe00ca4fa43e63efba1163356864
- Diff:
-
Revision 7 (+246 -32)
-
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 -
-
-
-
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
- Testing Done:
-
~ Unit test have been written to make sure that Capabilities are registered and unregistered properly. The third unit test also checks that none of the default capabilities can be changed or removed.
~ 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.
- Commit:
-
e1222f79007d47c47fb6bdf86cbddab80ff14323cf9eaa019fea8f710337430c713de7419ccd5a5f
-
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
-
-
-
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.
-
-
That alignment violates PEP-8. Instead, do:
WebAPICapabilitiesHook( self, { 'commit_ids': True, 'tested': True, }) -
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:: javascriptabove it. -
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.
-
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
-
Almost done! Couple small things.
-
-
-
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
SampleExtensionIDone, can you pretty-print it?
So:
"capabilities": { ... "SampleExtensionID": { "commit_ids": True, "tested": True } } -
-
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.
-
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
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