Allow Web API Capabilities Integration through ReviewBoard Extensions
Review Request #6613 — Created Nov. 21, 2014 and submitted
| Information | |
|---|---|
| justy777 | |
| Review Board | |
| master | |
| 6641 | |
| b112f63... | |
| Reviewers | |
| 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, }) … |
 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 |
-
-
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.
-
-
-
reviewboard/webapi/server_info.py (Diff revision 1) This will override
_capabilities_defaultsevery time. We instead need to make a copy. -
-
-
reviewboard/webapi/server_info.py (Diff revision 1) This should be a
ValueError, since it's the value that's wrong. -
reviewboard/webapi/server_info.py (Diff revision 1) This will fail if
capabilities_idis not in_registered_capabilities. Instead, this should be:if capabilities_id in _registred_capabilities:Same below.
-
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 2 (+135 -30) |
-
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
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+154 -30) |
-
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: |
|
|---|
Description: |
|
||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||||||||
Diff: |
Revision 4 (+189 -30) |
-
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: |
|
|||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||
Testing Done: |
|
|||||||||||||||
Commit: |
|
|||||||||||||||
Diff: |
Revision 5 (+225 -30) |
-
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
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 6 (+225 -30) |
-
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
-
-
docs/manual/extending/extensions/hooks/webapi-capability-hook.rst (Diff revision 6) 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?
-
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.
-
reviewboard/extensions/hooks.py (Diff revision 6) We should call this
WebAPICapabilitiesHook, since it handles more than one capability at a time. -
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.
-
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.
-
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 isNone. -
-
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(). -
reviewboard/webapi/server_info.py (Diff revision 6) How about "is reserved for the default set of capabilities" ?
-
reviewboard/webapi/server_info.py (Diff revision 6) This will be faster as:
try: del .... except KeyError: logging.... raise ...
Summary: |
|
||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||
Commit: |
|
||||||||||||
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 -
-
reviewboard/webapi/resources/validate_diff.py (Diff revision 7) Col: 22 E128 continuation line under-indented for visual indent
-
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 8 (+208 -30) |
-
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: |
|
||||||
|---|---|---|---|---|---|---|---|
Commit: |
|
||||||
Diff: |
Revision 9 (+272 -30) |
-
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
-
-
docs/manual/extending/extensions/hooks/webapi-capabilities-hook.rst (Diff revision 9) WebAPICapabilitiesHook -
docs/manual/extending/extensions/hooks/webapi-capabilities-hook.rst (Diff revision 9) 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.
-
docs/manual/extending/extensions/hooks/webapi-capabilities-hook.rst (Diff revision 9) This isn't a class, and it's an internal detail that we don't want to expose in the documentation.
-
docs/manual/extending/extensions/hooks/webapi-capabilities-hook.rst (Diff revision 9) That alignment violates PEP-8. Instead, do:
WebAPICapabilitiesHook( self, { 'commit_ids': True, 'tested': True, }) -
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:: javascriptabove it. -
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.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 10 (+283 -30) |
-
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.
-
docs/manual/extending/extensions/hooks/webapi-capabilities-hook.rst (Diff revision 10) Missing space before "The".
-
-
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
SampleExtensionIDone, can you pretty-print it?
So:
"capabilities": { ... "SampleExtensionID": { "commit_ids": True, "tested": True } } -
reviewboard/extensions/hooks.py (Diff revision 10) It never will be in practice, so no need to check this.
-
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.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 11 (+278 -30) |
-
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