-
-
-
reviewboard/webapi/base.py (Diff revision 1) Col: 19 E702 multiple statements on one line (semicolon)
-
-
Allow and abstract private extra data in the API
Review Request #7674 — Created Oct. 2, 2015 and submitted
The webapi allows for arbitrary data to be set in
extra_data
for certain API resources. Currently, all of that information is accessible through the API which is not always desirable.
The change in progress will allow users to specify privateextra_data
which will be abstracted from the publicly api available information.Change prohibits any kind of modification to private variables (denoted by a leading double-undersore
__
) in resources containing extra_data, as well as removing the private fields when a resource is requested.Added
APIExtraDataHook
to allow extension writers to give access states to extra_data fields.
Added new PUT, POST and GET tests for
__
denoted private extra_data.
Ran all tests.Added new tests for testing the
APIExtraDataHook
.
Description | From | Last Updated |
---|---|---|
Col: 35 E703 statement ends with a semicolon |
![]() |
|
Col: 19 E702 multiple statements on one line (semicolon) |
![]() |
|
Col: 19 E231 missing whitespace after ';' |
![]() |
|
Col: 32 E713 test for membership should be 'not in' |
![]() |
|
Col: 35 E703 statement ends with a semicolon |
![]() |
|
Col: 19 E702 multiple statements on one line (semicolon) |
![]() |
|
Col: 19 E231 missing whitespace after ';' |
![]() |
|
Col: 32 E713 test for membership should be 'not in' |
![]() |
|
Col: 35 E703 statement ends with a semicolon |
![]() |
|
Col: 19 E702 multiple statements on one line (semicolon) |
![]() |
|
Col: 19 E231 missing whitespace after ';' |
![]() |
|
Col: 19 E702 multiple statements on one line (semicolon) |
![]() |
|
Col: 19 E702 multiple statements on one line (semicolon) |
![]() |
|
Col: 19 E702 multiple statements on one line (semicolon) |
![]() |
|
Col: 19 E702 multiple statements on one line (semicolon) |
![]() |
|
Col: 19 E702 multiple statements on one line (semicolon) |
![]() |
|
Col: 39 E203 whitespace before ':' |
![]() |
|
Col: 19 E702 multiple statements on one line (semicolon) |
![]() |
|
Col: 1 W293 blank line contains whitespace |
![]() |
|
When we wrap conditionals, instead of using \, we like to surround the whole thing in parens. |
|
|
Long docstrings should have a single summary line, followed by a blank line, followed by more detailed description. Please also … |
|
|
This function doesn't seem to return anything? |
|
|
redefinition of unused 'test_post_with_extra_fields' from line 7 |
![]() |
|
Col: 39 E203 whitespace before ':' |
![]() |
|
Col: 39 E203 whitespace before ':' |
![]() |
|
Col: 21 W503 line break before binary operator |
![]() |
|
This funciton body should still be empty. |
|
|
This method should be called serialize_extra_data. That way, when serializing any field called extra_data, the web API will know to … |
|
|
Given what I said above, we shouldnt need this function. |
|
|
This should be a method on the class because it doesn't need closure around any variables. |
|
|
Blank line between these. |
|
|
Instead of POSTing, can we set the extra_data to a nested structure and do a GET test instead? |
|
|
How about: "Strip private fields from an extra data object." |
|
|
through the provided object |
|
|
"The object ..." |
|
|
You don't need the hasattr call. This function will only be called when there is an extra_data field. Also, if … |
|
|
Col: 19 W291 trailing whitespace |
![]() |
|
The not should be aligned with the self. (I know the pep8 tool will complain here, but we ignore this … |
|
|
So, this is going to actually modify the object itself, which is bad if the object then gets saved later. … |
|
|
No comma needed before the "as well as" |
CH chronicleyu | |
Should end in a period. |
|
|
Should be on previous line. |
|
|
Blank line between these. |
|
|
"Return" (imperitive mood) |
|
|
Reformat this as: key_path (tuple): A tuple of strings... |
|
|
unicode |
|
|
Missing a type. in the case of unions, (type or type or type ... or type). |
|
|
'APIExtraDataAccessHook' imported but unused |
![]() |
|
Blank line between these. |
|
|
This should probably be: super(APIExtraDataAccessHookTests, self).tearDown() |
|
|
This should go with the standard library imports. |
|
|
Single quotes |
|
|
This should probably be set in __init__. |
|
|
I believe you mean (self.EXTRA_DATA_ACCESS_PUBLIC, None) |
|
|
"Return" |
|
|
See above formatting comment. |
|
|
unicode |
|
|
No comma here. |
|
|
We can avoid a backslash here: url, mimetype, data, objs = self.setup_basic_post_test( self.user, False, None, True) |
|
|
Should be: unicode: The acess state of the provided field. |
|
|
Blank line between these. |
|
|
Blank line between these. |
|
|
Don't do import inside of a class. Do it inside defs if you need it locally |
|
|
'ExtensionInfo' imported but unused |
![]() |
|
Col: 80 E501 line too long (93 > 79 characters) |
![]() |
|
Col: 5 E303 too many blank lines (2) |
![]() |
|
Col: 5 E303 too many blank lines (2) |
![]() |
|
Col: 33 E203 whitespace before ':' |
![]() |
|
Col: 34 E203 whitespace before ':' |
![]() |
|
Col: 35 E203 whitespace before ':' |
![]() |
|
Needs a docstring. |
|
|
Col: 80 E501 line too long (87 > 79 characters) |
![]() |
|
local variable 'root' is assigned to but never used |
![]() |
|
Col: 9 E303 too many blank lines (2) |
![]() |
|
Col: 19 E702 multiple statements on one line (semicolon) |
![]() |
|
Can you seperate the constants from the overridden class attributes with a newline? |
|
|
Each of these should have a doc comment, e.g.: #: A public ``extra_data`` access state indicates everyone(?) can retrieve and … |
|
|
Remove this line. |
|
|
This code is very highly nested. Can we break this out into another function, e.g., _process_extra_data ? |
|
|
Just unicode: .... |
|
|
We probably don't want to call callback twice. Instead, we should cache the return value of it, e.g. value = … |
|
|
Blank line between these. |
|
|
If there is no access state, shouldn't it be public or something? I might be misunderstanding this. It should probably … |
|
|
How about "The object from which private fields will be stripped" |
|
|
Needs a docstring. |
|
|
Doing list(six.iteritems(d)) will remove all the benefits of doing six.iteritems in the first place. In Python 2, d.items() generates a … |
|
|
We avoid the Python ternary expression. Rewrite this as: if parent_path: constructed_path = parent_path + (k,) else: constructed_path = k |
|
|
Needs a docstring. |
|
|
Col: 5 E303 too many blank lines (2) |
![]() |
|
Col: 5 E303 too many blank lines (2) |
![]() |
|
Col: 33 E203 whitespace before ':' |
![]() |
|
Col: 34 E203 whitespace before ':' |
![]() |
|
Col: 35 E203 whitespace before ':' |
![]() |
|
Col: 80 E501 line too long (87 > 79 characters) |
![]() |
|
Col: 19 E702 multiple statements on one line (semicolon) |
![]() |
|
local variable 'rsp' is assigned to but never used |
![]() |
|
This should live in strip_private_data. If a subclass overwrites it and wants to implement the behaviour, it should use super. |
|
|
Can we rename k to field_name and v to value? |
|
|
My previous comment here wasn't clear and I didn't notice the dictionary modification while iterating issue. However, instead of using … |
|
|
What if you get rid of registration entirely? From looking at djblets/extensions/manager.py, it looks like if it doesn't find one, … |
|
|
Col: 5 E303 too many blank lines (2) |
![]() |
|
Col: 5 E303 too many blank lines (2) |
![]() |
|
Col: 33 E203 whitespace before ':' |
![]() |
|
Col: 34 E203 whitespace before ':' |
![]() |
|
Col: 35 E203 whitespace before ':' |
![]() |
|
Col: 9 E303 too many blank lines (2) |
![]() |
|
local variable 'accesshook' is assigned to but never used |
![]() |
|
local variable 'extension' is assigned to but never used |
![]() |
|
Col: 19 E702 multiple statements on one line (semicolon) |
![]() |
|
local variable 'rsp' is assigned to but never used |
![]() |
|
Col: 5 E303 too many blank lines (2) |
![]() |
|
Col: 5 E303 too many blank lines (2) |
![]() |
|
Col: 33 E203 whitespace before ':' |
![]() |
|
Col: 34 E203 whitespace before ':' |
![]() |
|
Col: 35 E203 whitespace before ':' |
![]() |
|
Col: 9 E303 too many blank lines (2) |
![]() |
|
local variable 'accesshook' is assigned to but never used |
![]() |
|
'ReviewRequestResource' imported but unused |
![]() |
|
Col: 80 E501 line too long (85 > 79 characters) |
![]() |
|
Col: 19 E702 multiple statements on one line (semicolon) |
![]() |
|
local variable 'rsp' is assigned to but never used |
![]() |
|
Col: 25 E225 missing whitespace around operator |
![]() |
|
Col: 29 E127 continuation line over-indented for visual indent |
![]() |
|
Col: 31 E231 missing whitespace after ':' |
![]() |
|
Col: 33 E203 whitespace before ':' |
![]() |
|
Col: 34 E203 whitespace before ':' |
![]() |
|
Col: 35 E203 whitespace before ':' |
![]() |
|
local variable 'accesshook' is assigned to but never used |
![]() |
|
local variable 'list_mimetype' is assigned to but never used |
![]() |
|
Col: 19 E702 multiple statements on one line (semicolon) |
![]() |
|
local variable 'rsp' is assigned to but never used |
![]() |
|
'augment_method_from' imported but unused |
![]() |
|
Col: 27 E702 multiple statements on one line (semicolon) |
![]() |
|
"A hook." |
|
|
Missing period. |
|
|
This should be the first funciton. |
|
|
this.class.path should be the actual class path, e.g. reviewboard.webapi.base.WebAPIResource |
|
|
This is probably unnecessary. |
|
|
Second line needs a colon. Comments like thsould be of the form of: #: Single line summary #: #: Multi-line … |
|
|
Same here. |
|
|
Same here. |
|
|
Since this is returning a boolean, its name should reflect that. Can we rename this to _should_processes_extra_data? |
|
|
Needs Args and Returns |
|
|
Needs docstring. |
|
|
Needs docstring |
|
|
Just "key path" |
|
|
Blank line between a statement and the start (or end) of a block. |
|
|
``extra_data`` Just two backslashes. Also, how about: A clone of the ``extra_data`` stripped of its private fields. |
|
|
Space between if and (. |
|
|
Can we add a helper function self._is_private(key) that just does return self.get_extra_data_field_state(path) == self.EXTRA_DATA_ACCESS_PRIVATE ? It will make this easier … |
|
|
Needs Args and Returns. |
|
|
Can you add some examples of nested extra data? e.g. extra_fields = { '__private_key': 'private_data', 'public_key': { '__another_private_key': 'foo', 'another_public_key': … |
|
|
Col: 16 E128 continuation line under-indented for visual indent |
![]() |
|
Col: 26 E127 continuation line over-indented for visual indent |
![]() |
|
This should be a lot more fleshed out, highlighting what kind of things can be set and how this impacts … |
|
|
You'll have to build the docs and see if these resolve with the line breaks. You can also prefix a … |
|
|
No need for enumerate here. That's only useful when you need the index, which you're not using. |
|
|
Needs a doc string. |
|
|
Must be in alphabetical order. |
|
|
I would declare this outside of setUp, because otherwise, the entire class is being re-created every time this is run. … |
|
|
Not too useful for the test. |
|
|
This needs to be a tuple. As it is, this is going to be treated as a tuple of ('G', … |
|
|
Docstrings aren't really necessary for test objects like this one. We'll never generate docs from them, and they're not useful … |
|
|
Should be formatted like: data = { 'extra_data': ... } |
|
|
I know other classes are doing all this. Instead of repeating all that, create a mixin to keep it all … |
|
|
This shouldn't be here. |
|
|
Should be one statement. |
|
|
Should be one statement. However, this sort of thing doesn't belong in setUp(), and instead should be in the tests … |
|
|
Multi-line dicts shoudl always be in the form of: extra_fields = { key: value, key: value, } Also, this can … |
|
|
Your tests here are doing too much. They're claiming to test registration, but then they're testing usage. Instead, you should … |
|
|
In this case, you should start arguments on the opening line. |
|
|
Should be part of the statement below. Having this in its own variable just reduces readability. |
|
|
Same as above. |
|
|
Same as above. |
|
|
Second line is indented too far. |
|
|
Let's pull these out into a dedicated class: ExtraDataAccessLevel. That will also help with the documentation, which won't have to … |
|
|
These must still follow the single-line summary, multi-line description format. |
|
|
Blank line between these. |
|
|
"if an ..." |
|
|
The descriptions belong on the following line, indented 4 spaces. |
|
|
No blank line here. Description should be on the following line. |
|
|
The second line should probably be indented 4 spaces, since otherwise it's a standalone condition per line. |
|
|
"Register" How about: "Register a function for determining access to an extra_data key." Same general idea in functions below. |
|
|
This is repeating the summary. Instead, flesh it out with more information about the purpose of registering such a thing. |
|
|
This should probably check that this wasn't already registered, and raise a ValueError with a suitable error message if so … |
|
|
This is repeating the summary. |
|
|
This can fail with a ValueError. Catch that and return a new ValueError with a more suitable error message (and … |
|
|
This should mention "extra_data", like the others. |
|
|
When adjusting the types above, this will be an int. |
|
|
Blank line between these. |
|
|
Too many backticks. Should just be 2. |
|
|
This needs a more specific name, and documentation. |
|
|
Public functions go above private ones. |
|
|
"Serialize" (no 's'), and "resource's". |
|
|
Description on the next line. |
|
|
The return type should come first, rather than being mentioned in parens. |
|
|
Your unit tests are only testing the __ ability. Instead, for each suitable HTTP method, you need to test: __ … |
|
|
Col: 80 E501 line too long (83 > 79 characters) |
![]() |
|
Col: 80 E501 line too long (95 > 79 characters) |
![]() |
|
Col: 13 E128 continuation line under-indented for visual indent |
![]() |
|
Col: 13 E128 continuation line under-indented for visual indent |
![]() |
|
Col: 13 E128 continuation line under-indented for visual indent |
![]() |
|
Col: 13 E128 continuation line under-indented for visual indent |
![]() |
|
Col: 13 E128 continuation line under-indented for visual indent |
![]() |
|
Col: 13 E128 continuation line under-indented for visual indent |
![]() |
|
Col: 13 E128 continuation line under-indented for visual indent |
![]() |
|
Col: 13 E128 continuation line under-indented for visual indent |
![]() |
|
Col: 12 E401 multiple imports on one line |
![]() |
|
Col: 21 E127 continuation line over-indented for visual indent |
![]() |
|
Col: 30 E127 continuation line over-indented for visual indent |
![]() |
|
Col: 30 E127 continuation line over-indented for visual indent |
![]() |
|
Col: 1 E303 too many blank lines (3) |
![]() |
|
Col: 29 W292 no newline at end of file |
![]() |
|
Col: 80 E501 line too long (83 > 79 characters) |
![]() |
|
Col: 80 E501 line too long (95 > 79 characters) |
![]() |
|
Col: 21 E127 continuation line over-indented for visual indent |
![]() |
|
Col: 1 E303 too many blank lines (3) |
![]() |
|
Col: 80 E501 line too long (83 > 79 characters) |
![]() |
|
Col: 80 E501 line too long (95 > 79 characters) |
![]() |
|
This should be done in setUp(), i.e., def setUp(self): super(ExtensionManagerMixin, self).setUp() self.manager = ExtensionManager('') |
|
|
Col: 21 E127 continuation line over-indented for visual indent |
![]() |
|
Can we call this register_extra_data_access_callback ? |
|
|
Likewise unregister_extra_data_access_callback |
|
|
Col: 80 E501 line too long (83 > 79 characters) |
![]() |
|
Col: 80 E501 line too long (95 > 79 characters) |
![]() |
|
Col: 5 E303 too many blank lines (2) |
![]() |
|
Col: 5 E303 too many blank lines (2) |
![]() |
|
Col: 28 E712 comparison to True should be 'if cond is True:' or 'if cond:' |
![]() |
|
Col: 80 E501 line too long (83 > 79 characters) |
![]() |
|
Col: 9 E303 too many blank lines (2) |
![]() |
|
Col: 21 E127 continuation line over-indented for visual indent |
![]() |
|
Too many spaces after "An". |
|
|
The PRIVATE, READ_ONLY, etc. don't mean much at this point when reading through the docs. I'd try rethinking how you … |
|
|
This isn't where they live now. |
|
|
This needs to point to the class owning the method. It should link properly when building the docs. |
|
|
provides -> provided |
CH chronicleyu | |
Two blank lines between sections. |
|
|
The source code should start 3 characters in, aligned with the code-block. |
|
|
No parens here. Instead, use \\. |
|
|
This isn't very readable. It should be: access_hook = APIExtraDataAccessHook( self, review_request_resource, [ (('private',), ExtraDataAccessLevel.ACCESS_STATE_PRIVATE), ]) (Note that it's not … |
|
|
This isn't valid Python, and we absolutely don't want to be documenting that monkey-patching a hook is valid. Instead, show … |
|
|
Typo on the hook variable name. |
|
|
No blank line at the end of the file. |
|
|
The wrapping is pretty funky in a lot of these. It should wrap close to the 79 character column. |
|
|
Col: 80 E501 line too long (86 > 79 characters) |
![]() |
|
This should be: ```python """ Example: ... code-block:: python resource.extra_Data = { ... """ |
|
|
Col: 80 E501 line too long (98 > 79 characters) |
![]() |
|
How does this look? It's a tuple, but we're comparing against the field_set. Can you show me an example of … |
|
|
None is a possible result, and should be documented. |
|
|
Docstring summaries cannot wrap. |
|
|
Blank line between these. |
|
|
No parens, and space after except. |
|
|
Alphabetical order (in the import ... list). |
|
|
This should have a docstring. |
|
|
Combine this to one statement. |
|
|
Should be: return 200, { 'test': self.extra_data, } |
|
|
There's no reason to have this function. |
|
|
Docstring summaries can't wrap, but in this case, don't bother with documenting this. It's not useful, since it's just a … |
|
|
Blank line between a class's docstring and its body. |
|
|
Here, too. |
|
|
Blank line between these. |
|
|
No trailing periods on the unit test docstrings (here and others). |
|
|
This (and others like it) are formatted awkwardly. Instead, try: APIExtraDataAccessHook( self.extension, self.resource, [ (('public',), ExtraDataAccessLevel.ACCESS_STATE_PUBLIC) ]) |
|
|
While assertEquals and assertNotEquals are valid aliases, we only use assertEqual and assertNotEqual. Make sure all the tests are using … |
|
|
A bunch of these are missing docstrings. |
|
|
Col: 21 E127 continuation line over-indented for visual indent |
![]() |
|
When viewing a log, this isn't going to mean much. I wouldn't log this. The caller will see the error. … |
|
|
callbable -> callable |
CH chronicleyu | |
This should provide an example of what's expected in key_path. Maybe have an "Example" section, like I document elsewhere. |
|
|
If this is meant only for internal use, it should have a _ prefix. |
|
|
Second line should not be indented. It should be on the same level as the prior line. |
|
|
serialize functions traditionally go higher up, below has_access_permissions. |
|
|
Is it worth having this method? It's just a simple comparison, and is only used in one place. Best to … |
|
|
Must inherit from object. Should also be placed in alphabetical order. |
|
|
Blank line between a class's docstring and its body. |
|
|
These must follow traditional docstring semantics of a summary line (max one sentence), blank line, description. |
|
|
Col: 80 E501 line too long (86 > 79 characters) |
![]() |
|
Col: 80 E501 line too long (98 > 79 characters) |
![]() |
|
Col: 21 E127 continuation line over-indented for visual indent |
![]() |
|
Col: 80 E501 line too long (86 > 79 characters) |
![]() |
|
Col: 80 E501 line too long (98 > 79 characters) |
![]() |
|
Col: 21 E127 continuation line over-indented for visual indent |
![]() |
|
indentation is off |
|


-
Tool: Pyflakes Processed Files: reviewboard/webapi/base.py reviewboard/webapi/resources/review_request_draft.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/base.py reviewboard/webapi/resources/review_request_draft.py
-
-
reviewboard/webapi/base.py (Diff revision 2) Col: 19 E702 multiple statements on one line (semicolon)
-
-
-
reviewboard/webapi/resources/review_request_draft.py (Diff revision 2) Col: 35 E703 statement ends with a semicolon
-
reviewboard/webapi/resources/review_request_draft.py (Diff revision 2) Col: 19 E702 multiple statements on one line (semicolon)
-
reviewboard/webapi/resources/review_request_draft.py (Diff revision 2) Col: 19 E231 missing whitespace after ';'

-
Tool: Pyflakes Processed Files: reviewboard/webapi/base.py reviewboard/webapi/resources/review_request_draft.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/base.py reviewboard/webapi/resources/review_request_draft.py
-
reviewboard/webapi/base.py (Diff revision 3) Col: 19 E702 multiple statements on one line (semicolon)
-
reviewboard/webapi/resources/review_request_draft.py (Diff revision 3) Col: 19 E702 multiple statements on one line (semicolon)

-
Tool: Pyflakes Processed Files: reviewboard/webapi/base.py reviewboard/webapi/resources/review_request_draft.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/base.py reviewboard/webapi/resources/review_request_draft.py
-
reviewboard/webapi/base.py (Diff revision 4) Col: 19 E702 multiple statements on one line (semicolon)
-
reviewboard/webapi/resources/review_request_draft.py (Diff revision 4) Col: 19 E702 multiple statements on one line (semicolon)
Description: |
|
---|
Description: |
|
---|

-
Tool: Pyflakes Processed Files: reviewboard/webapi/base.py reviewboard/webapi/resources/review_request_draft.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/base.py reviewboard/webapi/resources/review_request_draft.py
-
reviewboard/webapi/base.py (Diff revision 5) Col: 19 E702 multiple statements on one line (semicolon)
-
-
reviewboard/webapi/resources/review_request_draft.py (Diff revision 5) Col: 19 E702 multiple statements on one line (semicolon)

-
Tool: Pyflakes Processed Files: reviewboard/webapi/base.py reviewboard/webapi/resources/review_request_draft.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/base.py reviewboard/webapi/resources/review_request_draft.py

-
Tool: Pyflakes Processed Files: reviewboard/webapi/base.py reviewboard/webapi/resources/review_request_draft.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/base.py reviewboard/webapi/resources/review_request_draft.py
-
reviewboard/webapi/resources/review_request_draft.py (Diff revision 7) Col: 1 W293 blank line contains whitespace

-
Tool: Pyflakes Processed Files: reviewboard/webapi/base.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/base.py

-
Tool: Pyflakes Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py
-
reviewboard/webapi/tests/mixins_extra_data.py (Diff revision 9) redefinition of unused 'test_post_with_extra_fields' from line 7
-
-
-
-
reviewboard/webapi/base.py (Diff revision 9) When we wrap conditionals, instead of using \, we like to surround the whole thing in parens.
-
reviewboard/webapi/base.py (Diff revision 9) Long docstrings should have a single summary line, followed by a blank line, followed by more detailed description.
Please also add "Args" and "Returns" sections to this (we're trying to add those throughout--you can search the code to see examples).
-

-
Tool: Pyflakes Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py
-

-
Tool: Pyflakes Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py
-
-
-
reviewboard/webapi/base.py (Diff revision 11) This method should be called
serialize_extra_data
. That way, when serializing any field calledextra_data
, the web API will know to call this function on it. -
reviewboard/webapi/base.py (Diff revision 11) Given what I said above, we shouldnt need this function.
-
reviewboard/webapi/base.py (Diff revision 11) This should be a method on the class because it doesn't need closure around any variables.
-
-
reviewboard/webapi/tests/mixins_extra_data.py (Diff revision 11) Instead of
POST
ing, can we set theextra_data
to a nested structure and do aGET
test instead?

-
Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py Tool: Pyflakes Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py
-
You should remove the djblets change from your description
-
reviewboard/webapi/base.py (Diff revision 12) How about: "Strip private fields from an extra data object."
-
-
-
reviewboard/webapi/base.py (Diff revision 12) You don't need the
hasattr
call. This function will only be called when there is anextra_data
field.Also, if
extra_data
isNone
, we should serialize it as{}
ornull
.
Description: |
|
||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 13 (+102 -1) |

-
Tool: Pyflakes Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py
-

-
Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py Tool: Pyflakes Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py
-
Looks good, but there's a subtle bug that can manifest with the approach. Shouldn't be terrible to fix, though.
You should also reach out to Steven MacLeod in Slack about this, because I know he had things he needed beyond a
__
prefix. Essentially, we'll want to allow subclasses to be able to make a determination as to whether a key should be private, even without the__
(so that later we can allow extensions to strip these out). -
reviewboard/webapi/base.py (Diff revision 14) The
not
should be aligned with theself.
(I know the pep8 tool will complain here, but we ignore this one because it's ridiculous, and not even a PEP-8 issue, more a personal preference of the tool author.)
-
reviewboard/webapi/base.py (Diff revision 14) So, this is going to actually modify the object itself, which is bad if the object then gets saved later. These functions should instead generate new dictionaries, returning those, instead of modifying the provided dictionary.

-
Tool: Pyflakes Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py

-
Tool: Pyflakes Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py reviewboard/extensions/hooks.py reviewboard/extensions/tests.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py reviewboard/extensions/hooks.py reviewboard/extensions/tests.py
-
Description: |
|
||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
-
-
-
-
-
-
reviewboard/extensions/hooks.py (Diff revision 16) Reformat this as:
key_path (tuple): A tuple of strings...
-
-
reviewboard/extensions/hooks.py (Diff revision 16) Missing a type. in the case of unions,
(type or type or type ... or type)
. -
-
-
-
-
reviewboard/webapi/base.py (Diff revision 16) I believe you mean
(self.EXTRA_DATA_ACCESS_PUBLIC, None)
-
-
-
-
-
reviewboard/webapi/tests/mixins_extra_data.py (Diff revision 16) We can avoid a backslash here:
url, mimetype, data, objs = self.setup_basic_post_test( self.user, False, None, True)
-
-
reviewboard/extensions/tests.py (Diff revision 16) This should probably be:
super(APIExtraDataAccessHookTests, self).tearDown()

-
Tool: Pyflakes Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py reviewboard/extensions/hooks.py reviewboard/extensions/tests.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py reviewboard/extensions/hooks.py reviewboard/extensions/tests.py
-
-
-
-
-
-
-
-
-
reviewboard/extensions/tests.py (Diff revision 17) local variable 'root' is assigned to but never used
-
-
reviewboard/extensions/tests.py (Diff revision 17) Col: 19 E702 multiple statements on one line (semicolon)
-
-
reviewboard/extensions/hooks.py (Diff revision 17) Should be:
unicode: The acess state of the provided field
. -
-
-
reviewboard/extensions/tests.py (Diff revision 17) Don't do
import
inside of aclass
. Do it insidedef
s if you need it locally -
-
reviewboard/webapi/base.py (Diff revision 17) Can you seperate the constants from the overridden class attributes with a newline?
-
reviewboard/webapi/base.py (Diff revision 17) Each of these should have a doc comment, e.g.:
#: A public ``extra_data`` access state indicates everyone(?) can retrieve and edit it. EXTRA_DATA_ACCESS_PUBLIC = 'PUBLIC' #: A readonly public ....
For longer doc comments, they should be formatted as docstrings, e.g.
#: Single line summary. #: #: Multi-line description. #: Description continued. thing = Foo()
-
-
reviewboard/webapi/base.py (Diff revision 17) This code is very highly nested. Can we break this out into another function, e.g.,
_process_extra_data
? -
-
reviewboard/webapi/base.py (Diff revision 17) We probably don't want to call
callback
twice. Instead, we should cache the return value of it, e.g.value = callback(key_path) if value is not None: return value
-
-
reviewboard/webapi/base.py (Diff revision 17) If there is no access state, shouldn't it be public or something?
I might be misunderstanding this. It should probably be documented.
-
reviewboard/webapi/base.py (Diff revision 17) How about "The object from which private fields will be stripped"
-
-
reviewboard/webapi/base.py (Diff revision 17) Doing
list(six.iteritems(d))
will remove all the benefits of doingsix.iteritems
in the first place.In Python 2,
d.items()
generates a list of key-value pairs. However, that is a waste of memory unless you are explicitly going to use it as a list. In the case of iteration, we generally want to usesix.iteritems(d)
. This uses a generator toyield
all the key-value pairs instead of constructing a list (and wasting memory by doing so).Doing
list(six.iteritems(d))
will construct a list from the generator, wasting as much memory as just doingd.items()
, but also being less efficient because there are going to be an an extraO(n)
function calls. -
reviewboard/webapi/base.py (Diff revision 17) We avoid the Python ternary expression.
Rewrite this as:
if parent_path: constructed_path = parent_path + (k,) else: constructed_path = k
-

-
Tool: Pyflakes Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py reviewboard/extensions/hooks.py reviewboard/extensions/tests.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py reviewboard/extensions/hooks.py reviewboard/extensions/tests.py
-
-
-
-
-
-
-
reviewboard/extensions/tests.py (Diff revision 18) Col: 19 E702 multiple statements on one line (semicolon)
-
reviewboard/extensions/tests.py (Diff revision 18) local variable 'rsp' is assigned to but never used
-
-
reviewboard/webapi/base.py (Diff revision 18) This should live in
strip_private_data
. If a subclass overwrites it and wants to implement the behaviour, it should use super. -
reviewboard/webapi/base.py (Diff revision 18) My previous comment here wasn't clear and I didn't notice the dictionary modification while iterating issue. However, instead of using
.items()
(which will blow up on Py3 because it is an iterator there), we can take a modified approach.Instead of passing a deep copy of
extra_data
tostrip_private_data
, we can instead pass the originalextra_data
and make the copy there. Then we can iterate through the originalextra_data
's keys and modify our copy.In essence:
def strip_private_data(self, extra_data, parent_path=None): clone = copy.copy(extra_data) for field_name, value in six.iteritems(extra_data): if parent_path: path = parent_path + (field_name,) else: path = field_name if (field_name.startswith(PRIVATE_KEY_PREFIX) or self.get_extra_data_field_state(constructed_path) == self.EXTRA_DATA_ACCESS_PRIVATE): del clone[field_name] elif isinstance(value, dict): clone[field_name] = self.strip_private_data(clone[field_name], path) return clone def seralize_extra_data_field: if obj.extra_data is not None: return self.strip_private_data(obj.extra_data) return None
We no longer require doing a
deepcopy
of theextra_data
becuase we do a shallow copy at each level.

-
Tool: Pyflakes Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py reviewboard/extensions/hooks.py reviewboard/extensions/tests.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py reviewboard/extensions/hooks.py reviewboard/extensions/tests.py
-
-
-
-
-
-
-
reviewboard/extensions/tests.py (Diff revision 19) local variable 'accesshook' is assigned to but never used
-
reviewboard/extensions/tests.py (Diff revision 19) local variable 'extension' is assigned to but never used
-
reviewboard/extensions/tests.py (Diff revision 19) Col: 19 E702 multiple statements on one line (semicolon)
-
reviewboard/extensions/tests.py (Diff revision 19) local variable 'rsp' is assigned to but never used
-
-
reviewboard/extensions/tests.py (Diff revision 19) What if you get rid of
registration
entirely? From looking at djblets/extensions/manager.py, it looks like if it doesn't find one, it'll create one itself. Looking at the other tests in that folder as well (like the "ExtensionManagerTest") the TestExtension doesn't define aregistration
member. So try leaving that out?

-
Tool: Pyflakes Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py reviewboard/extensions/hooks.py reviewboard/extensions/tests.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py reviewboard/extensions/hooks.py reviewboard/extensions/tests.py
-
-
-
-
-
-
-
reviewboard/extensions/tests.py (Diff revision 20) local variable 'accesshook' is assigned to but never used
-
-
-
reviewboard/extensions/tests.py (Diff revision 20) Col: 19 E702 multiple statements on one line (semicolon)
-
reviewboard/extensions/tests.py (Diff revision 20) local variable 'rsp' is assigned to but never used

-
Tool: Pyflakes Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py reviewboard/extensions/hooks.py reviewboard/extensions/tests.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py reviewboard/extensions/hooks.py reviewboard/extensions/tests.py
-
-
reviewboard/extensions/tests.py (Diff revision 21) Col: 29 E127 continuation line over-indented for visual indent
-
-
-
-
-
reviewboard/extensions/tests.py (Diff revision 21) local variable 'accesshook' is assigned to but never used
-
reviewboard/extensions/tests.py (Diff revision 21) local variable 'list_mimetype' is assigned to but never used
-
reviewboard/extensions/tests.py (Diff revision 21) Col: 19 E702 multiple statements on one line (semicolon)
-
reviewboard/extensions/tests.py (Diff revision 21) local variable 'rsp' is assigned to but never used

-
Tool: Pyflakes Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py reviewboard/extensions/hooks.py reviewboard/extensions/tests.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py reviewboard/extensions/hooks.py reviewboard/extensions/tests.py
-
-
reviewboard/extensions/tests.py (Diff revision 22) Col: 27 E702 multiple statements on one line (semicolon)
Description: |
|
||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|

-
Tool: Pyflakes Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py reviewboard/extensions/hooks.py reviewboard/extensions/tests.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py reviewboard/extensions/hooks.py reviewboard/extensions/tests.py
-
-
-
-
-
reviewboard/extensions/hooks.py (Diff revision 23) this.class.path
should be the actual class path, e.g.reviewboard.webapi.base.WebAPIResource
-
-
reviewboard/webapi/base.py (Diff revision 23) Second line needs a colon. Comments like thsould be of the form of:
#: Single line summary #: #: Multi-line description
just like a docstring.
-
-
-
reviewboard/webapi/base.py (Diff revision 23) Since this is returning a boolean, its name should reflect that.
Can we rename this to
_should_processes_extra_data
? -
-
-
-
-
reviewboard/webapi/base.py (Diff revision 23) Blank line between a statement and the start (or end) of a block.
-
reviewboard/webapi/base.py (Diff revision 23) ``extra_data``
Just two backslashes.
Also, how about:
A clone of the ``extra_data`` stripped of its private fields.
-
-
reviewboard/webapi/base.py (Diff revision 23) Can we add a helper function
self._is_private(key)
that just doesreturn self.get_extra_data_field_state(path) == self.EXTRA_DATA_ACCESS_PRIVATE
?It will make this easier to read.
-
-
reviewboard/webapi/tests/mixins_extra_data.py (Diff revision 23) Can you add some examples of nested extra data?
e.g.
extra_fields = { '__private_key': 'private_data', 'public_key': { '__another_private_key': 'foo', 'another_public_key': 'bar', }, }
And verify that works as intended.

-
Tool: Pyflakes Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py reviewboard/extensions/hooks.py reviewboard/extensions/tests.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py reviewboard/extensions/hooks.py reviewboard/extensions/tests.py
-
reviewboard/webapi/base.py (Diff revision 24) Col: 16 E128 continuation line under-indented for visual indent
-
reviewboard/webapi/tests/mixins_extra_data.py (Diff revision 24) Col: 26 E127 continuation line over-indented for visual indent

-
Tool: Pyflakes Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py reviewboard/extensions/hooks.py reviewboard/extensions/tests.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py reviewboard/extensions/hooks.py reviewboard/extensions/tests.py
-
This is looking great. I do have a lot of comments (I know that wall of yellow looks scary), but most are wording changes in docs or little things that need changing. There are a couple bigger-ticket items for how some stuff is structured/named, but you'll plow through those fast. The biggest is that the test coverage isn't complete, so I'll want to see some more things there (I outline this below). Having that coverage will help prove your implementation.
-
reviewboard/extensions/hooks.py (Diff revision 25) This should be a lot more fleshed out, highlighting what kind of things can be set and how this impacts the API. This will be used as a base for documentation on the hook.
-
reviewboard/extensions/hooks.py (Diff revision 25) You'll have to build the docs and see if these resolve with the line breaks.
You can also prefix a module path like this with
~
so that the generated docs only show the last component of the path. -
reviewboard/extensions/hooks.py (Diff revision 25) No need for
enumerate
here. That's only useful when you need the index, which you're not using. -
-
-
reviewboard/extensions/tests.py (Diff revision 25) I would declare this outside of
setUp
, because otherwise, the entire class is being re-created every time this is run. Top-level is fine, if you name the class right. -
-
reviewboard/extensions/tests.py (Diff revision 25) This needs to be a tuple. As it is, this is going to be treated as a tuple of
('G', 'E', 'T')
-
reviewboard/extensions/tests.py (Diff revision 25) Docstrings aren't really necessary for test objects like this one. We'll never generate docs from them, and they're not useful for the test.
-
reviewboard/extensions/tests.py (Diff revision 25) Should be formatted like:
data = { 'extra_data': ... }
-
reviewboard/extensions/tests.py (Diff revision 25) I know other classes are doing all this. Instead of repeating all that, create a mixin to keep it all in one place.
-
-
-
reviewboard/extensions/tests.py (Diff revision 25) Should be one statement.
However, this sort of thing doesn't belong in
setUp()
, and instead should be in the tests that use them. The reason being that as the test suite grows, having these pre-generated data sets will become less useful and more confusing (a contributor won't know if they should be using it, adding more pre-generated data sets for their tests, etc.). -
reviewboard/extensions/tests.py (Diff revision 25) Multi-line dicts shoudl always be in the form of:
extra_fields = { key: value, key: value, }
Also, this can definitely be merged with the statement below.
-
reviewboard/extensions/tests.py (Diff revision 25) Your tests here are doing too much. They're claiming to test registration, but then they're testing usage.
Instead, you should have a test for each of these conditions:
- Registration (just check that the resulting function is properly registered)
- Unregistration (just check that the formerly-registered function is now unregistered)
- The hook usage with a key set for the public state.
- The hook usage with a key set for the read-only state (and attempting to modify).
- The hook usage with a key set for the private state.
Fine-grained tests are better than larger, more encompassing tests, because when things go wrong, we can see if only a subset of the tests fail. This gives the developer a better idea of what broke, without having to start debugging the code.
-
reviewboard/extensions/tests.py (Diff revision 25) In this case, you should start arguments on the opening line.
-
reviewboard/extensions/tests.py (Diff revision 25) Should be part of the statement below. Having this in its own variable just reduces readability.
-
-
-
-
reviewboard/webapi/base.py (Diff revision 25) Let's pull these out into a dedicated class:
ExtraDataAccessLevel
.That will also help with the documentation, which won't have to describe that it's an "extra_data access state" over and over.
I'd also just simply use integers for the level types. They're smaller for storage, and faster to compare.
-
reviewboard/webapi/base.py (Diff revision 25) These must still follow the single-line summary, multi-line description format.
-
-
-
reviewboard/webapi/base.py (Diff revision 25) The descriptions belong on the following line, indented 4 spaces.
-
reviewboard/webapi/base.py (Diff revision 25) No blank line here.
Description should be on the following line.
-
reviewboard/webapi/base.py (Diff revision 25) The second line should probably be indented 4 spaces, since otherwise it's a standalone condition per line.
-
reviewboard/webapi/base.py (Diff revision 25) "Register"
How about: "Register a function for determining access to an extra_data key."
Same general idea in functions below.
-
reviewboard/webapi/base.py (Diff revision 25) This is repeating the summary. Instead, flesh it out with more information about the purpose of registering such a thing.
-
reviewboard/webapi/base.py (Diff revision 25) This should probably check that this wasn't already registered, and raise a
ValueError
with a suitable error message if so (which will also need to be documented).It should also check that what's provided is a
callable
. -
-
reviewboard/webapi/base.py (Diff revision 25) This can fail with a
ValueError
. Catch that and return a newValueError
with a more suitable error message (and document it).It should also check that what's provided is a
callable
. -
-
-
-
-
-
-
-
-
reviewboard/webapi/base.py (Diff revision 25) The return type should come first, rather than being mentioned in parens.
-
reviewboard/webapi/tests/mixins_extra_data.py (Diff revision 25) Your unit tests are only testing the
__
ability. Instead, for each suitable HTTP method, you need to test:__
prefixes on paths- Registered extra_data access functions returning "public".
- Registered extra_data access functions returning "read-only".
- Registered extra_data access functions returning "private".
You also need unit tests covering the register/unregister functions with the various success/fail conditions, and
get_extra_data_field_state
with the various conditions. These should go in a more dedicated unit test function, rather than being in this mixin, because we don't need to test those functions a hundred times.

-
Tool: Pyflakes Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py reviewboard/extensions/hooks.py reviewboard/extensions/tests.py Ignored Files: docs/manual/extending/extensions/hooks/index.rst docs/manual/extending/extensions/hooks/api-extra-data-access-hook.rst Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py reviewboard/extensions/hooks.py reviewboard/extensions/tests.py Ignored Files: docs/manual/extending/extensions/hooks/index.rst docs/manual/extending/extensions/hooks/api-extra-data-access-hook.rst
-
-
-
reviewboard/extensions/tests.py (Diff revision 26) Col: 13 E128 continuation line under-indented for visual indent
-
reviewboard/extensions/tests.py (Diff revision 26) Col: 13 E128 continuation line under-indented for visual indent
-
reviewboard/extensions/tests.py (Diff revision 26) Col: 13 E128 continuation line under-indented for visual indent
-
reviewboard/extensions/tests.py (Diff revision 26) Col: 13 E128 continuation line under-indented for visual indent
-
reviewboard/extensions/tests.py (Diff revision 26) Col: 13 E128 continuation line under-indented for visual indent
-
reviewboard/extensions/tests.py (Diff revision 26) Col: 13 E128 continuation line under-indented for visual indent
-
reviewboard/extensions/tests.py (Diff revision 26) Col: 13 E128 continuation line under-indented for visual indent
-
reviewboard/extensions/tests.py (Diff revision 26) Col: 13 E128 continuation line under-indented for visual indent
-
-
reviewboard/webapi/base.py (Diff revision 26) Col: 21 E127 continuation line over-indented for visual indent
-
reviewboard/webapi/base.py (Diff revision 26) Col: 30 E127 continuation line over-indented for visual indent
-
reviewboard/webapi/base.py (Diff revision 26) Col: 30 E127 continuation line over-indented for visual indent
-
-

-
Tool: Pyflakes Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py reviewboard/extensions/hooks.py reviewboard/extensions/tests.py Ignored Files: docs/manual/extending/extensions/hooks/index.rst docs/manual/extending/extensions/hooks/api-extra-data-access-hook.rst Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py reviewboard/extensions/hooks.py reviewboard/extensions/tests.py Ignored Files: docs/manual/extending/extensions/hooks/index.rst docs/manual/extending/extensions/hooks/api-extra-data-access-hook.rst
-
-
-
reviewboard/webapi/base.py (Diff revision 27) Col: 21 E127 continuation line over-indented for visual indent
-

-
Tool: Pyflakes Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py reviewboard/extensions/hooks.py reviewboard/extensions/tests.py Ignored Files: docs/manual/extending/extensions/hooks/index.rst docs/manual/extending/extensions/hooks/api-extra-data-access-hook.rst Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py reviewboard/extensions/hooks.py reviewboard/extensions/tests.py Ignored Files: docs/manual/extending/extensions/hooks/index.rst docs/manual/extending/extensions/hooks/api-extra-data-access-hook.rst
-
-
-
reviewboard/webapi/base.py (Diff revision 28) Col: 21 E127 continuation line over-indented for visual indent
-
-
reviewboard/extensions/tests.py (Diff revision 28) This should be done in
setUp()
, i.e.,def setUp(self): super(ExtensionManagerMixin, self).setUp() self.manager = ExtensionManager('')
-
reviewboard/webapi/base.py (Diff revision 28) Can we call this
register_extra_data_access_callback
? -

-
Tool: Pyflakes Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py reviewboard/extensions/hooks.py reviewboard/extensions/tests.py Ignored Files: docs/manual/extending/extensions/hooks/index.rst docs/manual/extending/extensions/hooks/api-extra-data-access-hook.rst Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/base.py reviewboard/webapi/tests/mixins_extra_data.py reviewboard/extensions/hooks.py reviewboard/extensions/tests.py Ignored Files: docs/manual/extending/extensions/hooks/index.rst docs/manual/extending/extensions/hooks/api-extra-data-access-hook.rst
-
-
-
-
-
reviewboard/extensions/tests.py (Diff revision 29) Col: 28 E712 comparison to True should be 'if cond is True:' or 'if cond:'
-