Allow and abstract private extra data in the API

Review Request #7674 — Created Oct. 2, 2015 and submitted

ahache
Review Board
release-2.5.x
8084
reviewboard, students

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 private extra_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

reviewbotreviewbot

Col: 19 E702 multiple statements on one line (semicolon)

reviewbotreviewbot

Col: 19 E231 missing whitespace after ';'

reviewbotreviewbot

Col: 32 E713 test for membership should be 'not in'

reviewbotreviewbot

Col: 35 E703 statement ends with a semicolon

reviewbotreviewbot

Col: 19 E702 multiple statements on one line (semicolon)

reviewbotreviewbot

Col: 19 E231 missing whitespace after ';'

reviewbotreviewbot

Col: 32 E713 test for membership should be 'not in'

reviewbotreviewbot

Col: 35 E703 statement ends with a semicolon

reviewbotreviewbot

Col: 19 E702 multiple statements on one line (semicolon)

reviewbotreviewbot

Col: 19 E231 missing whitespace after ';'

reviewbotreviewbot

Col: 19 E702 multiple statements on one line (semicolon)

reviewbotreviewbot

Col: 19 E702 multiple statements on one line (semicolon)

reviewbotreviewbot

Col: 19 E702 multiple statements on one line (semicolon)

reviewbotreviewbot

Col: 19 E702 multiple statements on one line (semicolon)

reviewbotreviewbot

Col: 19 E702 multiple statements on one line (semicolon)

reviewbotreviewbot

Col: 39 E203 whitespace before ':'

reviewbotreviewbot

Col: 19 E702 multiple statements on one line (semicolon)

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

When we wrap conditionals, instead of using \, we like to surround the whole thing in parens.

daviddavid

Long docstrings should have a single summary line, followed by a blank line, followed by more detailed description. Please also ...

daviddavid

This function doesn't seem to return anything?

daviddavid

redefinition of unused 'test_post_with_extra_fields' from line 7

reviewbotreviewbot

Col: 39 E203 whitespace before ':'

reviewbotreviewbot

Col: 39 E203 whitespace before ':'

reviewbotreviewbot

Col: 21 W503 line break before binary operator

reviewbotreviewbot

This funciton body should still be empty.

brenniebrennie

This method should be called serialize_extra_data. That way, when serializing any field called extra_data, the web API will know to ...

brenniebrennie

Given what I said above, we shouldnt need this function.

brenniebrennie

This should be a method on the class because it doesn't need closure around any variables.

brenniebrennie

Blank line between these.

brenniebrennie

Instead of POSTing, can we set the extra_data to a nested structure and do a GET test instead?

brenniebrennie

How about: "Strip private fields from an extra data object."

brenniebrennie

through the provided object

brenniebrennie

"The object ..."

brenniebrennie

You don't need the hasattr call. This function will only be called when there is an extra_data field. Also, if ...

brenniebrennie

Col: 19 W291 trailing whitespace

reviewbotreviewbot

The not should be aligned with the self. (I know the pep8 tool will complain here, but we ignore this ...

chipx86chipx86

So, this is going to actually modify the object itself, which is bad if the object then gets saved later. ...

chipx86chipx86

No comma needed before the "as well as"

CH chronicleyu

Should end in a period.

brenniebrennie

Should be on previous line.

brenniebrennie

Blank line between these.

brenniebrennie

"Return" (imperitive mood)

brenniebrennie

Reformat this as: key_path (tuple): A tuple of strings...

brenniebrennie

unicode

brenniebrennie

Missing a type. in the case of unions, (type or type or type ... or type).

brenniebrennie

'APIExtraDataAccessHook' imported but unused

reviewbotreviewbot

Blank line between these.

brenniebrennie

This should probably be: super(APIExtraDataAccessHookTests, self).tearDown()

mike_conleymike_conley

This should go with the standard library imports.

brenniebrennie

Single quotes

brenniebrennie

This should probably be set in __init__.

brenniebrennie

I believe you mean (self.EXTRA_DATA_ACCESS_PUBLIC, None)

brenniebrennie

"Return"

brenniebrennie

See above formatting comment.

brenniebrennie

unicode

brenniebrennie

No comma here.

brenniebrennie

We can avoid a backslash here: url, mimetype, data, objs = self.setup_basic_post_test( self.user, False, None, True)

brenniebrennie

Should be: unicode: The acess state of the provided field.

brenniebrennie

Blank line between these.

brenniebrennie

Blank line between these.

brenniebrennie

Don't do import inside of a class. Do it inside defs if you need it locally

brenniebrennie

'ExtensionInfo' imported but unused

reviewbotreviewbot

Col: 80 E501 line too long (93 > 79 characters)

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

Col: 33 E203 whitespace before ':'

reviewbotreviewbot

Col: 34 E203 whitespace before ':'

reviewbotreviewbot

Col: 35 E203 whitespace before ':'

reviewbotreviewbot

Needs a docstring.

brenniebrennie

Col: 80 E501 line too long (87 > 79 characters)

reviewbotreviewbot

local variable 'root' is assigned to but never used

reviewbotreviewbot

Col: 9 E303 too many blank lines (2)

reviewbotreviewbot

Col: 19 E702 multiple statements on one line (semicolon)

reviewbotreviewbot

Can you seperate the constants from the overridden class attributes with a newline?

brenniebrennie

Each of these should have a doc comment, e.g.: #: A public ``extra_data`` access state indicates everyone(?) can retrieve and ...

brenniebrennie

Remove this line.

brenniebrennie

This code is very highly nested. Can we break this out into another function, e.g., _process_extra_data ?

brenniebrennie

Just unicode: ....

brenniebrennie

We probably don't want to call callback twice. Instead, we should cache the return value of it, e.g. value = ...

brenniebrennie

Blank line between these.

brenniebrennie

If there is no access state, shouldn't it be public or something? I might be misunderstanding this. It should probably ...

brenniebrennie

How about "The object from which private fields will be stripped"

brenniebrennie

Needs a docstring.

brenniebrennie

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

brenniebrennie

We avoid the Python ternary expression. Rewrite this as: if parent_path: constructed_path = parent_path + (k,) else: constructed_path = k

brenniebrennie

Needs a docstring.

brenniebrennie

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

Col: 33 E203 whitespace before ':'

reviewbotreviewbot

Col: 34 E203 whitespace before ':'

reviewbotreviewbot

Col: 35 E203 whitespace before ':'

reviewbotreviewbot

Col: 80 E501 line too long (87 > 79 characters)

reviewbotreviewbot

Col: 19 E702 multiple statements on one line (semicolon)

reviewbotreviewbot

local variable 'rsp' is assigned to but never used

reviewbotreviewbot

This should live in strip_private_data. If a subclass overwrites it and wants to implement the behaviour, it should use super.

brenniebrennie

Can we rename k to field_name and v to value?

brenniebrennie

My previous comment here wasn't clear and I didn't notice the dictionary modification while iterating issue. However, instead of using ...

brenniebrennie

What if you get rid of registration entirely? From looking at djblets/extensions/manager.py, it looks like if it doesn't find one, ...

mike_conleymike_conley

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

Col: 33 E203 whitespace before ':'

reviewbotreviewbot

Col: 34 E203 whitespace before ':'

reviewbotreviewbot

Col: 35 E203 whitespace before ':'

reviewbotreviewbot

Col: 9 E303 too many blank lines (2)

reviewbotreviewbot

local variable 'accesshook' is assigned to but never used

reviewbotreviewbot

local variable 'extension' is assigned to but never used

reviewbotreviewbot

Col: 19 E702 multiple statements on one line (semicolon)

reviewbotreviewbot

local variable 'rsp' is assigned to but never used

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

Col: 33 E203 whitespace before ':'

reviewbotreviewbot

Col: 34 E203 whitespace before ':'

reviewbotreviewbot

Col: 35 E203 whitespace before ':'

reviewbotreviewbot

Col: 9 E303 too many blank lines (2)

reviewbotreviewbot

local variable 'accesshook' is assigned to but never used

reviewbotreviewbot

'ReviewRequestResource' imported but unused

reviewbotreviewbot

Col: 80 E501 line too long (85 > 79 characters)

reviewbotreviewbot

Col: 19 E702 multiple statements on one line (semicolon)

reviewbotreviewbot

local variable 'rsp' is assigned to but never used

reviewbotreviewbot

Col: 25 E225 missing whitespace around operator

reviewbotreviewbot

Col: 29 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 31 E231 missing whitespace after ':'

reviewbotreviewbot

Col: 33 E203 whitespace before ':'

reviewbotreviewbot

Col: 34 E203 whitespace before ':'

reviewbotreviewbot

Col: 35 E203 whitespace before ':'

reviewbotreviewbot

local variable 'accesshook' is assigned to but never used

reviewbotreviewbot

local variable 'list_mimetype' is assigned to but never used

reviewbotreviewbot

Col: 19 E702 multiple statements on one line (semicolon)

reviewbotreviewbot

local variable 'rsp' is assigned to but never used

reviewbotreviewbot

'augment_method_from' imported but unused

reviewbotreviewbot

Col: 27 E702 multiple statements on one line (semicolon)

reviewbotreviewbot

"A hook."

brenniebrennie

Missing period.

brenniebrennie

This should be the first funciton.

brenniebrennie

this.class.path should be the actual class path, e.g. reviewboard.webapi.base.WebAPIResource

brenniebrennie

This is probably unnecessary.

brenniebrennie

Second line needs a colon. Comments like thsould be of the form of: #: Single line summary #: #: Multi-line ...

brenniebrennie

Same here.

brenniebrennie

Same here.

brenniebrennie

Since this is returning a boolean, its name should reflect that. Can we rename this to _should_processes_extra_data?

brenniebrennie

Needs Args and Returns

brenniebrennie

Needs docstring.

brenniebrennie

Needs docstring

brenniebrennie

Just "key path"

brenniebrennie

Blank line between a statement and the start (or end) of a block.

brenniebrennie

``extra_data`` Just two backslashes. Also, how about: A clone of the ``extra_data`` stripped of its private fields.

brenniebrennie

Space between if and (.

brenniebrennie

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

brenniebrennie

Needs Args and Returns.

brenniebrennie

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': ...

brenniebrennie

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

reviewbotreviewbot

Col: 26 E127 continuation line over-indented for visual indent

reviewbotreviewbot

This should be a lot more fleshed out, highlighting what kind of things can be set and how this impacts ...

chipx86chipx86

You'll have to build the docs and see if these resolve with the line breaks. You can also prefix a ...

chipx86chipx86

No need for enumerate here. That's only useful when you need the index, which you're not using.

chipx86chipx86

Needs a doc string.

chipx86chipx86

Must be in alphabetical order.

chipx86chipx86

I would declare this outside of setUp, because otherwise, the entire class is being re-created every time this is run. ...

chipx86chipx86

Not too useful for the test.

chipx86chipx86

This needs to be a tuple. As it is, this is going to be treated as a tuple of ('G', ...

chipx86chipx86

Docstrings aren't really necessary for test objects like this one. We'll never generate docs from them, and they're not useful ...

chipx86chipx86

Should be formatted like: data = { 'extra_data': ... }

chipx86chipx86

I know other classes are doing all this. Instead of repeating all that, create a mixin to keep it all ...

chipx86chipx86

This shouldn't be here.

chipx86chipx86

Should be one statement.

chipx86chipx86

Should be one statement. However, this sort of thing doesn't belong in setUp(), and instead should be in the tests ...

chipx86chipx86

Multi-line dicts shoudl always be in the form of: extra_fields = { key: value, key: value, } Also, this can ...

chipx86chipx86

Your tests here are doing too much. They're claiming to test registration, but then they're testing usage. Instead, you should ...

chipx86chipx86

In this case, you should start arguments on the opening line.

chipx86chipx86

Should be part of the statement below. Having this in its own variable just reduces readability.

chipx86chipx86

Same as above.

chipx86chipx86

Same as above.

chipx86chipx86

Second line is indented too far.

chipx86chipx86

Let's pull these out into a dedicated class: ExtraDataAccessLevel. That will also help with the documentation, which won't have to ...

chipx86chipx86

These must still follow the single-line summary, multi-line description format.

chipx86chipx86

Blank line between these.

chipx86chipx86

"if an ..."

chipx86chipx86

The descriptions belong on the following line, indented 4 spaces.

chipx86chipx86

No blank line here. Description should be on the following line.

chipx86chipx86

The second line should probably be indented 4 spaces, since otherwise it's a standalone condition per line.

chipx86chipx86

"Register" How about: "Register a function for determining access to an extra_data key." Same general idea in functions below.

chipx86chipx86

This is repeating the summary. Instead, flesh it out with more information about the purpose of registering such a thing.

chipx86chipx86

This should probably check that this wasn't already registered, and raise a ValueError with a suitable error message if so ...

chipx86chipx86

This is repeating the summary.

chipx86chipx86

This can fail with a ValueError. Catch that and return a new ValueError with a more suitable error message (and ...

chipx86chipx86

This should mention "extra_data", like the others.

chipx86chipx86

When adjusting the types above, this will be an int.

chipx86chipx86

Blank line between these.

chipx86chipx86

Too many backticks. Should just be 2.

chipx86chipx86

This needs a more specific name, and documentation.

chipx86chipx86

Public functions go above private ones.

chipx86chipx86

"Serialize" (no 's'), and "resource's".

chipx86chipx86

Description on the next line.

chipx86chipx86

The return type should come first, rather than being mentioned in parens.

chipx86chipx86

Your unit tests are only testing the __ ability. Instead, for each suitable HTTP method, you need to test: __ ...

chipx86chipx86

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (95 > 79 characters)

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 12 E401 multiple imports on one line

reviewbotreviewbot

Col: 21 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 30 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 30 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 1 E303 too many blank lines (3)

reviewbotreviewbot

Col: 29 W292 no newline at end of file

reviewbotreviewbot

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (95 > 79 characters)

reviewbotreviewbot

Col: 21 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 1 E303 too many blank lines (3)

reviewbotreviewbot

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (95 > 79 characters)

reviewbotreviewbot

This should be done in setUp(), i.e., def setUp(self): super(ExtensionManagerMixin, self).setUp() self.manager = ExtensionManager('')

brenniebrennie

Col: 21 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Can we call this register_extra_data_access_callback ?

brenniebrennie

Likewise unregister_extra_data_access_callback

brenniebrennie

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (95 > 79 characters)

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

Col: 28 E712 comparison to True should be 'if cond is True:' or 'if cond:'

reviewbotreviewbot

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

Col: 9 E303 too many blank lines (2)

reviewbotreviewbot

Col: 21 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Too many spaces after "An".

chipx86chipx86

The PRIVATE, READ_ONLY, etc. don't mean much at this point when reading through the docs. I'd try rethinking how you ...

chipx86chipx86

This isn't where they live now.

chipx86chipx86

This needs to point to the class owning the method. It should link properly when building the docs.

chipx86chipx86

provides -> provided

CH chronicleyu

Two blank lines between sections.

chipx86chipx86

The source code should start 3 characters in, aligned with the code-block.

chipx86chipx86

No parens here. Instead, use \\.

chipx86chipx86

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

chipx86chipx86

This isn't valid Python, and we absolutely don't want to be documenting that monkey-patching a hook is valid. Instead, show ...

chipx86chipx86

Typo on the hook variable name.

chipx86chipx86

No blank line at the end of the file.

chipx86chipx86

The wrapping is pretty funky in a lot of these. It should wrap close to the 79 character column.

chipx86chipx86

Col: 80 E501 line too long (86 > 79 characters)

reviewbotreviewbot

This should be: ```python """ Example: ... code-block:: python resource.extra_Data = { ... """

chipx86chipx86

Col: 80 E501 line too long (98 > 79 characters)

reviewbotreviewbot

How does this look? It's a tuple, but we're comparing against the field_set. Can you show me an example of ...

chipx86chipx86

None is a possible result, and should be documented.

chipx86chipx86

Docstring summaries cannot wrap.

chipx86chipx86

Blank line between these.

chipx86chipx86

No parens, and space after except.

chipx86chipx86

Alphabetical order (in the import ... list).

chipx86chipx86

This should have a docstring.

chipx86chipx86

Combine this to one statement.

chipx86chipx86

Should be: return 200, { 'test': self.extra_data, }

chipx86chipx86

There's no reason to have this function.

chipx86chipx86

Docstring summaries can't wrap, but in this case, don't bother with documenting this. It's not useful, since it's just a ...

chipx86chipx86

Blank line between a class's docstring and its body.

chipx86chipx86

Here, too.

chipx86chipx86

Blank line between these.

chipx86chipx86

No trailing periods on the unit test docstrings (here and others).

chipx86chipx86

This (and others like it) are formatted awkwardly. Instead, try: APIExtraDataAccessHook( self.extension, self.resource, [ (('public',), ExtraDataAccessLevel.ACCESS_STATE_PUBLIC) ])

chipx86chipx86

While assertEquals and assertNotEquals are valid aliases, we only use assertEqual and assertNotEqual. Make sure all the tests are using ...

chipx86chipx86

A bunch of these are missing docstrings.

chipx86chipx86

Col: 21 E127 continuation line over-indented for visual indent

reviewbotreviewbot

When viewing a log, this isn't going to mean much. I wouldn't log this. The caller will see the error. ...

chipx86chipx86

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.

chipx86chipx86

If this is meant only for internal use, it should have a _ prefix.

chipx86chipx86

Second line should not be indented. It should be on the same level as the prior line.

chipx86chipx86

serialize functions traditionally go higher up, below has_access_permissions.

chipx86chipx86

Is it worth having this method? It's just a simple comparison, and is only used in one place. Best to ...

chipx86chipx86

Must inherit from object. Should also be placed in alphabetical order.

chipx86chipx86

Blank line between a class's docstring and its body.

chipx86chipx86

These must follow traditional docstring semantics of a summary line (max one sentence), blank line, description.

chipx86chipx86

Col: 80 E501 line too long (86 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (98 > 79 characters)

reviewbotreviewbot

Col: 21 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 80 E501 line too long (86 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (98 > 79 characters)

reviewbotreviewbot

Col: 21 E127 continuation line over-indented for visual indent

reviewbotreviewbot

indentation is off

brenniebrennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/base.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/base.py
    
    
  2. reviewboard/webapi/base.py (Diff revision 1)
     
     
    Col: 35
     E703 statement ends with a semicolon
    
  3. reviewboard/webapi/base.py (Diff revision 1)
     
     
    Col: 19
     E702 multiple statements on one line (semicolon)
    
  4. reviewboard/webapi/base.py (Diff revision 1)
     
     
    Col: 19
     E231 missing whitespace after ';'
    
  5. reviewboard/webapi/base.py (Diff revision 1)
     
     
    Col: 32
     E713 test for membership should be 'not in'
    
  6. 
      
AH
reviewbot
  1. 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
    
    
  2. reviewboard/webapi/base.py (Diff revision 2)
     
     
    Col: 35
     E703 statement ends with a semicolon
    
  3. reviewboard/webapi/base.py (Diff revision 2)
     
     
    Col: 19
     E702 multiple statements on one line (semicolon)
    
  4. reviewboard/webapi/base.py (Diff revision 2)
     
     
    Col: 19
     E231 missing whitespace after ';'
    
  5. reviewboard/webapi/base.py (Diff revision 2)
     
     
    Col: 32
     E713 test for membership should be 'not in'
    
  6. Col: 35
     E703 statement ends with a semicolon
    
  7. Col: 19
     E702 multiple statements on one line (semicolon)
    
  8. Col: 19
     E231 missing whitespace after ';'
    
  9. 
      
AH
reviewbot
  1. 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
    
    
  2. reviewboard/webapi/base.py (Diff revision 3)
     
     
    Col: 19
     E702 multiple statements on one line (semicolon)
    
  3. Col: 19
     E702 multiple statements on one line (semicolon)
    
  4. 
      
AH
reviewbot
  1. 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
    
    
  2. reviewboard/webapi/base.py (Diff revision 4)
     
     
    Col: 19
     E702 multiple statements on one line (semicolon)
    
  3. Col: 19
     E702 multiple statements on one line (semicolon)
    
  4. 
      
AH
AH
AH
AH
reviewbot
  1. 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
    
    
  2. reviewboard/webapi/base.py (Diff revision 5)
     
     
    Col: 19
     E702 multiple statements on one line (semicolon)
    
  3. reviewboard/webapi/base.py (Diff revision 5)
     
     
    Col: 39
     E203 whitespace before ':'
    
  4. Col: 19
     E702 multiple statements on one line (semicolon)
    
  5. 
      
AH
reviewbot
  1. 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
    
    
  2. 
      
AH
reviewbot
  1. 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
    
    
  2. Col: 1
     W293 blank line contains whitespace
    
  3. 
      
AH
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/base.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/base.py
    
    
  2. 
      
AH
reviewbot
  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
    
    
  2.  redefinition of unused 'test_post_with_extra_fields' from line 7
    
  3. Col: 39
     E203 whitespace before ':'
    
  4. Col: 39
     E203 whitespace before ':'
    
  5. 
      
david
  1. 
      
  2. reviewboard/webapi/base.py (Diff revision 9)
     
     
     

    When we wrap conditionals, instead of using \, we like to surround the whole thing in parens.

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

  4. reviewboard/webapi/base.py (Diff revision 9)
     
     
     
     

    This function doesn't seem to return anything?

    1. It doesn't return anything no; however, it does strip the private data out of extra_data, and api_data has a reference to it.
      Is this acceptable this way or should it be changed so that the function has a return value?

    2. OK, I think I understand better now. Perhaps rename extract_extra_data to find_extra_data, and then add some comments explaining that strip_extra_data manipulates the dictionary in-place?

    3. Oh ok, sounds good. Now there is no extract_extra_data function anymore, just strip_private_data and serialize_extra_data_field. But I'll add additional comments to the strip_extra_data to mention that it modifies the provided dictionary object in place.

  5. 
      
AH
reviewbot
  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
    
    
  2. reviewboard/webapi/base.py (Diff revision 10)
     
     
    Col: 21
     W503 line break before binary operator
    
  3. 
      
AH
reviewbot
  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
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/webapi/base.py (Diff revision 11)
     
     

    This funciton body should still be empty.

  3. reviewboard/webapi/base.py (Diff revision 11)
     
     

    This method should be called serialize_extra_data. That way, when serializing any field called extra_data, the web API will know to call this function on it.

    1. So, by calling it serialize_extra_data this will get called auto-magically? Man. Well, that's awesome, I'll do that then.

  4. reviewboard/webapi/base.py (Diff revision 11)
     
     
     
     
     
     
     

    Given what I said above, we shouldnt need this function.

  5. reviewboard/webapi/base.py (Diff revision 11)
     
     
     
     
     
     
     

    This should be a method on the class because it doesn't need closure around any variables.

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

    Blank line between these.

  7. reviewboard/webapi/tests/mixins_extra_data.py (Diff revision 11)
     
     
     
     

    Instead of POSTing, can we set the extra_data to a nested structure and do a GET test instead?

    1. As in, not having the 'private data' be at the top level of extra_data? i.e., have it nested into another field? If that's what you mean, I did try doing that at first but I couldn't get it work. I asked Christian about it and he said "we don't actually have a format today for setting nested data through the API for extra_data".
      And I'm about to work on a GET test right now, do we only want a GET test or do we want to keep the POST and PUT tests as well?

  8. 
      
AH
reviewbot
  1. 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
    
    
  2. 
      
AH
brennie
  1. You should remove the djblets change from your description

  2. reviewboard/webapi/base.py (Diff revision 12)
     
     

    How about: "Strip private fields from an extra data object."

  3. reviewboard/webapi/base.py (Diff revision 12)
     
     

    through the provided object
    
  4. reviewboard/webapi/base.py (Diff revision 12)
     
     

    "The object ..."

  5. reviewboard/webapi/base.py (Diff revision 12)
     
     

    You don't need the hasattr call. This function will only be called when there is an extra_data field.

    Also, if extra_data is None, we should serialize it as {} or null.

  6. 
      
AH
reviewbot
  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
    
    
  2. reviewboard/webapi/base.py (Diff revision 13)
     
     
    Col: 19
     W291 trailing whitespace
    
  3. 
      
AH
reviewbot
  1. 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
    
    
  2. 
      
chipx86
  1. 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).

    1. Will do! I'll see what he has to suggest about it then.

  2. reviewboard/webapi/base.py (Diff revision 14)
     
     
     

    The not should be aligned with the self.

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

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

  4. 
      
AH
reviewbot
  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
    
    
  2. 
      
CH
  1. 
      
  2. reviewboard/webapi/base.py (Diff revision 15)
     
     

    No comma needed before the "as well as"

  3. 
      
AH
reviewbot
  1. 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
    
    
  2. reviewboard/extensions/tests.py (Diff revision 16)
     
     
     'APIExtraDataAccessHook' imported but unused
    
  3. 
      
AH
brennie
  1. 
      
  2. reviewboard/extensions/hooks.py (Diff revision 16)
     
     

    Should end in a period.

  3. reviewboard/extensions/hooks.py (Diff revision 16)
     
     

    Should be on previous line.

  4. reviewboard/extensions/hooks.py (Diff revision 16)
     
     
     

    Blank line between these.

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

    "Return" (imperitive mood)

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

    Reformat this as:

    key_path (tuple):
        A tuple of strings...
    
  7. reviewboard/extensions/hooks.py (Diff revision 16)
     
     

    unicode

  8. reviewboard/extensions/hooks.py (Diff revision 16)
     
     

    Missing a type. in the case of unions, (type or type or type ... or type).

  9. reviewboard/extensions/tests.py (Diff revision 16)
     
     
     

    Blank line between these.

  10. reviewboard/webapi/base.py (Diff revision 16)
     
     

    This should go with the standard library imports.

  11. reviewboard/webapi/base.py (Diff revision 16)
     
     
     
     

    Single quotes

  12. reviewboard/webapi/base.py (Diff revision 16)
     
     

    This should probably be set in __init__.

    1. WebAPIResource doesn't have an __init__ function, so I tried to create one and add self._extra_data_access_callbacks = [] but then that caused 1392 tests to fail. So maybe it should stay out of the init function? Unless I did something wrong.

    2. You can create one, but you'll need to call the super class's __init__ from it.

    3. Derp. However, I just added super(WebAPIResource, self).__init__() as the first line in the init, and it still resulted in all the tests failing.

  13. reviewboard/webapi/base.py (Diff revision 16)
     
     

    I believe you mean (self.EXTRA_DATA_ACCESS_PUBLIC, None)

  14. reviewboard/webapi/base.py (Diff revision 16)
     
     

    "Return"

  15. reviewboard/webapi/base.py (Diff revision 16)
     
     
     

    See above formatting comment.

  16. reviewboard/webapi/base.py (Diff revision 16)
     
     

    unicode

  17. reviewboard/webapi/base.py (Diff revision 16)
     
     

    No comma here.

  18. 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)
    
  19. 
      
mike_conley
  1. 
      
  2. reviewboard/extensions/tests.py (Diff revision 16)
     
     

    This should probably be:

    super(APIExtraDataAccessHookTests, self).tearDown()
    
  3. 
      
AH
reviewbot
  1. 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
    
    
  2. reviewboard/extensions/tests.py (Diff revision 17)
     
     
     'ExtensionInfo' imported but unused
    
  3. reviewboard/extensions/tests.py (Diff revision 17)
     
     
    Col: 80
     E501 line too long (93 > 79 characters)
    
  4. reviewboard/extensions/tests.py (Diff revision 17)
     
     
    Col: 5
     E303 too many blank lines (2)
    
  5. reviewboard/extensions/tests.py (Diff revision 17)
     
     
    Col: 5
     E303 too many blank lines (2)
    
  6. reviewboard/extensions/tests.py (Diff revision 17)
     
     
    Col: 33
     E203 whitespace before ':'
    
  7. reviewboard/extensions/tests.py (Diff revision 17)
     
     
    Col: 34
     E203 whitespace before ':'
    
  8. reviewboard/extensions/tests.py (Diff revision 17)
     
     
    Col: 35
     E203 whitespace before ':'
    
  9. reviewboard/extensions/tests.py (Diff revision 17)
     
     
    Col: 80
     E501 line too long (87 > 79 characters)
    
  10. reviewboard/extensions/tests.py (Diff revision 17)
     
     
     local variable 'root' is assigned to but never used
    
  11. reviewboard/extensions/tests.py (Diff revision 17)
     
     
    Col: 9
     E303 too many blank lines (2)
    
  12. reviewboard/extensions/tests.py (Diff revision 17)
     
     
    Col: 19
     E702 multiple statements on one line (semicolon)
    
  13. 
      
brennie
  1. 
      
  2. reviewboard/extensions/hooks.py (Diff revision 17)
     
     

    Should be:

    unicode: The acess state of the provided field.

  3. reviewboard/extensions/hooks.py (Diff revision 17)
     
     
     

    Blank line between these.

  4. reviewboard/extensions/hooks.py (Diff revision 17)
     
     
     

    Blank line between these.

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

    Don't do import inside of a class. Do it inside defs if you need it locally

  6. reviewboard/extensions/tests.py (Diff revision 17)
     
     

    Needs a docstring.

  7. reviewboard/webapi/base.py (Diff revision 17)
     
     
     

    Can you seperate the constants from the overridden class attributes with a newline?

  8. 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()
    
  9. reviewboard/webapi/base.py (Diff revision 17)
     
     

    Remove this line.

  10. 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 ?

  11. reviewboard/webapi/base.py (Diff revision 17)
     
     

    Just unicode: ....

  12. 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
    
  13. reviewboard/webapi/base.py (Diff revision 17)
     
     
     

    Blank line between these.

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

    1. We could return public ya. But since fields are public by default, maybe we don't really need a public access state? Opinions?

  15. reviewboard/webapi/base.py (Diff revision 17)
     
     

    How about "The object from which private fields will be stripped"

  16. reviewboard/webapi/base.py (Diff revision 17)
     
     

    Needs a docstring.

  17. reviewboard/webapi/base.py (Diff revision 17)
     
     

    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 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 use six.iteritems(d). This uses a generator to yield 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 doing d.items(), but also being less efficient because there are going to be an an extra O(n) function calls.

    1. Ok I see, I had initially done six.iteritems but that would give me a dictionary changed size during iteration error. So I'll change it to use extra_data.items() instead.

  18. 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
    
  19. reviewboard/webapi/base.py (Diff revision 17)
     
     

    Needs a docstring.

  20. 
      
AH
reviewbot
  1. 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
    
    
  2. reviewboard/extensions/tests.py (Diff revision 18)
     
     
    Col: 5
     E303 too many blank lines (2)
    
  3. reviewboard/extensions/tests.py (Diff revision 18)
     
     
    Col: 5
     E303 too many blank lines (2)
    
  4. reviewboard/extensions/tests.py (Diff revision 18)
     
     
    Col: 33
     E203 whitespace before ':'
    
  5. reviewboard/extensions/tests.py (Diff revision 18)
     
     
    Col: 34
     E203 whitespace before ':'
    
  6. reviewboard/extensions/tests.py (Diff revision 18)
     
     
    Col: 35
     E203 whitespace before ':'
    
  7. reviewboard/extensions/tests.py (Diff revision 18)
     
     
    Col: 80
     E501 line too long (87 > 79 characters)
    
  8. reviewboard/extensions/tests.py (Diff revision 18)
     
     
    Col: 19
     E702 multiple statements on one line (semicolon)
    
  9. reviewboard/extensions/tests.py (Diff revision 18)
     
     
     local variable 'rsp' is assigned to but never used
    
  10. 
      
brennie
  1. 
      
  2. 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.

  3. 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 to strip_private_data, we can instead pass the original extra_data and make the copy there. Then we can iterate through the original extra_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 the extra_data becuase we do a shallow copy at each level.

  4. 
      
brennie
  1. 
      
  2. reviewboard/webapi/base.py (Diff revision 18)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Can we rename k to field_name and v to value?

  3. 
      
AH
reviewbot
  1. 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
    
    
  2. reviewboard/extensions/tests.py (Diff revision 19)
     
     
    Col: 5
     E303 too many blank lines (2)
    
  3. reviewboard/extensions/tests.py (Diff revision 19)
     
     
    Col: 5
     E303 too many blank lines (2)
    
  4. reviewboard/extensions/tests.py (Diff revision 19)
     
     
    Col: 33
     E203 whitespace before ':'
    
  5. reviewboard/extensions/tests.py (Diff revision 19)
     
     
    Col: 34
     E203 whitespace before ':'
    
  6. reviewboard/extensions/tests.py (Diff revision 19)
     
     
    Col: 35
     E203 whitespace before ':'
    
  7. reviewboard/extensions/tests.py (Diff revision 19)
     
     
    Col: 9
     E303 too many blank lines (2)
    
  8. reviewboard/extensions/tests.py (Diff revision 19)
     
     
     local variable 'accesshook' is assigned to but never used
    
  9. reviewboard/extensions/tests.py (Diff revision 19)
     
     
     local variable 'extension' is assigned to but never used
    
  10. reviewboard/extensions/tests.py (Diff revision 19)
     
     
    Col: 19
     E702 multiple statements on one line (semicolon)
    
  11. reviewboard/extensions/tests.py (Diff revision 19)
     
     
     local variable 'rsp' is assigned to but never used
    
  12. 
      
mike_conley
  1. 
      
  2. 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 a registration member. So try leaving that out?

    1. I started without adding the registration attribute. However when it's omitted, an error is raised when the extension is initialized during instantiating.

    2. What is the error?

    3. Ah sorry I had posted it on Slack.

      ERROR: Testing APIExtraDataAccessHook registration.

      Traceback (most recent call last):
      File "/home/tones/git/reviewboard/reviewboard/extensions/tests.py", line 630, in setUp
      self.extension = self.ext_cls(extension_manager=self.manager)
      File "/home/tones/git/reviewboard/reviewboard/extensions/tests.py", line 590, in init
      self).init(extension_manager, args, *kwargs)
      File "/home/tones/git/djblets/djblets/extensions/extension.py", line 104, in init
      self.settings = Settings(self)
      File "/home/tones/git/djblets/djblets/extensions/settings.py", line 46, in init
      self.load()
      File "/home/tones/git/djblets/djblets/extensions/settings.py", line 107, in load
      self.update(self.extension.registration.settings)
      AttributeError: 'ResourceTestExtension' object has no attribute 'registration'


  3. 
      
AH
reviewbot
  1. 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
    
    
  2. reviewboard/extensions/tests.py (Diff revision 20)
     
     
    Col: 5
     E303 too many blank lines (2)
    
  3. reviewboard/extensions/tests.py (Diff revision 20)
     
     
    Col: 5
     E303 too many blank lines (2)
    
  4. reviewboard/extensions/tests.py (Diff revision 20)
     
     
    Col: 33
     E203 whitespace before ':'
    
  5. reviewboard/extensions/tests.py (Diff revision 20)
     
     
    Col: 34
     E203 whitespace before ':'
    
  6. reviewboard/extensions/tests.py (Diff revision 20)
     
     
    Col: 35
     E203 whitespace before ':'
    
  7. reviewboard/extensions/tests.py (Diff revision 20)
     
     
    Col: 9
     E303 too many blank lines (2)
    
  8. reviewboard/extensions/tests.py (Diff revision 20)
     
     
     local variable 'accesshook' is assigned to but never used
    
  9. reviewboard/extensions/tests.py (Diff revision 20)
     
     
     'ReviewRequestResource' imported but unused
    
  10. reviewboard/extensions/tests.py (Diff revision 20)
     
     
    Col: 80
     E501 line too long (85 > 79 characters)
    
  11. reviewboard/extensions/tests.py (Diff revision 20)
     
     
    Col: 19
     E702 multiple statements on one line (semicolon)
    
  12. reviewboard/extensions/tests.py (Diff revision 20)
     
     
     local variable 'rsp' is assigned to but never used
    
  13. 
      
AH
reviewbot
  1. 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
    
    
  2. reviewboard/extensions/tests.py (Diff revision 21)
     
     
    Col: 25
     E225 missing whitespace around operator
    
  3. reviewboard/extensions/tests.py (Diff revision 21)
     
     
    Col: 29
     E127 continuation line over-indented for visual indent
    
  4. reviewboard/extensions/tests.py (Diff revision 21)
     
     
    Col: 31
     E231 missing whitespace after ':'
    
  5. reviewboard/extensions/tests.py (Diff revision 21)
     
     
    Col: 33
     E203 whitespace before ':'
    
  6. reviewboard/extensions/tests.py (Diff revision 21)
     
     
    Col: 34
     E203 whitespace before ':'
    
  7. reviewboard/extensions/tests.py (Diff revision 21)
     
     
    Col: 35
     E203 whitespace before ':'
    
  8. reviewboard/extensions/tests.py (Diff revision 21)
     
     
     local variable 'accesshook' is assigned to but never used
    
  9. reviewboard/extensions/tests.py (Diff revision 21)
     
     
     local variable 'list_mimetype' is assigned to but never used
    
  10. reviewboard/extensions/tests.py (Diff revision 21)
     
     
    Col: 19
     E702 multiple statements on one line (semicolon)
    
  11. reviewboard/extensions/tests.py (Diff revision 21)
     
     
     local variable 'rsp' is assigned to but never used
    
  12. 
      
AH
reviewbot
  1. 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
    
    
  2. reviewboard/extensions/tests.py (Diff revision 22)
     
     
     'augment_method_from' imported but unused
    
  3. reviewboard/extensions/tests.py (Diff revision 22)
     
     
    Col: 27
     E702 multiple statements on one line (semicolon)
    
  4. 
      
AH
AH
reviewbot
  1. 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
    
    
  2. 
      
AH
brennie
  1. 
      
  2. reviewboard/extensions/hooks.py (Diff revision 23)
     
     

    "A hook."

  3. reviewboard/extensions/hooks.py (Diff revision 23)
     
     

    Missing period.

  4. reviewboard/extensions/hooks.py (Diff revision 23)
     
     

    This should be the first funciton.

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

    this.class.path should be the actual class path, e.g.

    reviewboard.webapi.base.WebAPIResource

  6. reviewboard/extensions/tests.py (Diff revision 23)
     
     

    This is probably unnecessary.

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

  8. reviewboard/webapi/base.py (Diff revision 23)
     
     
     

    Same here.

  9. reviewboard/webapi/base.py (Diff revision 23)
     
     
     

    Same here.

  10. 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?

  11. reviewboard/webapi/base.py (Diff revision 23)
     
     

    Needs Args and Returns

  12. reviewboard/webapi/base.py (Diff revision 23)
     
     

    Needs docstring.

  13. reviewboard/webapi/base.py (Diff revision 23)
     
     

    Needs docstring

  14. reviewboard/webapi/base.py (Diff revision 23)
     
     

    Just "key path"

  15. reviewboard/webapi/base.py (Diff revision 23)
     
     
     

    Blank line between a statement and the start (or end) of a block.

  16. 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.
    
  17. reviewboard/webapi/base.py (Diff revision 23)
     
     

    Space between if and (.

  18. reviewboard/webapi/base.py (Diff revision 23)
     
     
     

    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 to read.

  19. reviewboard/webapi/base.py (Diff revision 23)
     
     

    Needs Args and Returns.

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

  21. 
      
AH
reviewbot
  1. 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
    
    
  2. reviewboard/webapi/base.py (Diff revision 24)
     
     
    Col: 16
     E128 continuation line under-indented for visual indent
    
  3. Col: 26
     E127 continuation line over-indented for visual indent
    
  4. 
      
AH
reviewbot
  1. 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
    
    
  2. 
      
chipx86
  1. 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.

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

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

    1. It resolves the path correctly, but it doesn't render the link. Do we let it exceed the 80 characters in a case like this? =/

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

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

    Needs a doc string.

  6. reviewboard/extensions/tests.py (Diff revision 25)
     
     

    Must be in alphabetical order.

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

  8. reviewboard/extensions/tests.py (Diff revision 25)
     
     
     

    Not too useful for the test.

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

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

  11. reviewboard/extensions/tests.py (Diff revision 25)
     
     
     

    Should be formatted like:

    data = {
        'extra_data': ...
    }
    
  12. 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.

  13. reviewboard/extensions/tests.py (Diff revision 25)
     
     
     
     

    This shouldn't be here.

  14. reviewboard/extensions/tests.py (Diff revision 25)
     
     
     

    Should be one statement.

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

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

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

  18. reviewboard/extensions/tests.py (Diff revision 25)
     
     
     

    In this case, you should start arguments on the opening line.

  19. reviewboard/extensions/tests.py (Diff revision 25)
     
     

    Should be part of the statement below. Having this in its own variable just reduces readability.

    1. I put this as an attribute in the resource itself now so that it's not repeated across all the new test cases I added.

  20. reviewboard/extensions/tests.py (Diff revision 25)
     
     
     

    Same as above.

  21. reviewboard/extensions/tests.py (Diff revision 25)
     
     

    Same as above.

  22. reviewboard/webapi/base.py (Diff revision 25)
     
     
     

    Second line is indented too far.

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

    1. Oh ok, that works. Where is the appropriate location to put this new class?

  24. reviewboard/webapi/base.py (Diff revision 25)
     
     
     

    These must still follow the single-line summary, multi-line description format.

  25. reviewboard/webapi/base.py (Diff revision 25)
     
     
     

    Blank line between these.

  26. reviewboard/webapi/base.py (Diff revision 25)
     
     

    "if an ..."

  27. reviewboard/webapi/base.py (Diff revision 25)
     
     
     
     

    The descriptions belong on the following line, indented 4 spaces.

  28. reviewboard/webapi/base.py (Diff revision 25)
     
     
     
     

    No blank line here.

    Description should be on the following line.

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

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

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

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

  33. reviewboard/webapi/base.py (Diff revision 25)
     
     
     

    This is repeating the summary.

  34. reviewboard/webapi/base.py (Diff revision 25)
     
     

    This can fail with a ValueError. Catch that and return a new ValueError with a more suitable error message (and document it).

    It should also check that what's provided is a callable.

  35. reviewboard/webapi/base.py (Diff revision 25)
     
     

    This should mention "extra_data", like the others.

  36. reviewboard/webapi/base.py (Diff revision 25)
     
     

    When adjusting the types above, this will be an int.

  37. reviewboard/webapi/base.py (Diff revision 25)
     
     
     

    Blank line between these.

  38. reviewboard/webapi/base.py (Diff revision 25)
     
     

    Too many backticks. Should just be 2.

  39. reviewboard/webapi/base.py (Diff revision 25)
     
     

    This needs a more specific name, and documentation.

  40. reviewboard/webapi/base.py (Diff revision 25)
     
     

    Public functions go above private ones.

  41. reviewboard/webapi/base.py (Diff revision 25)
     
     

    "Serialize" (no 's'), and "resource's".

  42. reviewboard/webapi/base.py (Diff revision 25)
     
     

    Description on the next line.

  43. reviewboard/webapi/base.py (Diff revision 25)
     
     

    The return type should come first, rather than being mentioned in parens.

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

  45. 
      
AH
reviewbot
  1. 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
    
    
  2. reviewboard/extensions/hooks.py (Diff revision 26)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  3. reviewboard/extensions/hooks.py (Diff revision 26)
     
     
    Col: 80
     E501 line too long (95 > 79 characters)
    
  4. reviewboard/extensions/tests.py (Diff revision 26)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  5. reviewboard/extensions/tests.py (Diff revision 26)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  6. reviewboard/extensions/tests.py (Diff revision 26)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  7. reviewboard/extensions/tests.py (Diff revision 26)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  8. reviewboard/extensions/tests.py (Diff revision 26)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  9. reviewboard/extensions/tests.py (Diff revision 26)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  10. reviewboard/extensions/tests.py (Diff revision 26)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  11. reviewboard/extensions/tests.py (Diff revision 26)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  12. reviewboard/webapi/base.py (Diff revision 26)
     
     
    Col: 12
     E401 multiple imports on one line
    
  13. reviewboard/webapi/base.py (Diff revision 26)
     
     
    Col: 21
     E127 continuation line over-indented for visual indent
    
  14. reviewboard/webapi/base.py (Diff revision 26)
     
     
    Col: 30
     E127 continuation line over-indented for visual indent
    
  15. reviewboard/webapi/base.py (Diff revision 26)
     
     
    Col: 30
     E127 continuation line over-indented for visual indent
    
  16. reviewboard/webapi/base.py (Diff revision 26)
     
     
    Col: 1
     E303 too many blank lines (3)
    
  17. reviewboard/webapi/base.py (Diff revision 26)
     
     
    Col: 29
     W292 no newline at end of file
    
  18. 
      
AH
reviewbot
  1. 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
    
    
  2. reviewboard/extensions/hooks.py (Diff revision 27)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  3. reviewboard/extensions/hooks.py (Diff revision 27)
     
     
    Col: 80
     E501 line too long (95 > 79 characters)
    
  4. reviewboard/webapi/base.py (Diff revision 27)
     
     
    Col: 21
     E127 continuation line over-indented for visual indent
    
  5. reviewboard/webapi/base.py (Diff revision 27)
     
     
    Col: 1
     E303 too many blank lines (3)
    
  6. 
      
AH
reviewbot
  1. 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
    
    
  2. reviewboard/extensions/hooks.py (Diff revision 28)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  3. reviewboard/extensions/hooks.py (Diff revision 28)
     
     
    Col: 80
     E501 line too long (95 > 79 characters)
    
    1. or should be on the next line technically

  4. reviewboard/webapi/base.py (Diff revision 28)
     
     
    Col: 21
     E127 continuation line over-indented for visual indent
    
    1. This shouldnt be indented.

    2. It wasn't indented before but Christian said that it should be indented?

  5. 
      
brennie
  1. 
      
  2. 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('')
    
  3. reviewboard/webapi/base.py (Diff revision 28)
     
     

    Can we call this register_extra_data_access_callback ?

  4. reviewboard/webapi/base.py (Diff revision 28)
     
     

    Likewise unregister_extra_data_access_callback

  5. 
      
AH
reviewbot
  1. 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
    
    
  2. reviewboard/extensions/hooks.py (Diff revision 29)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  3. reviewboard/extensions/hooks.py (Diff revision 29)
     
     
    Col: 80
     E501 line too long (95 > 79 characters)
    
  4. reviewboard/extensions/tests.py (Diff revision 29)
     
     
    Col: 5
     E303 too many blank lines (2)
    
  5. reviewboard/extensions/tests.py (Diff revision 29)
     
     
    Col: 5
     E303 too many blank lines (2)
    
  6. reviewboard/extensions/tests.py (Diff revision 29)
     
     
    Col: 28
     E712 comparison to True should be 'if cond is True:' or 'if cond:'
    
  7. reviewboard/extensions/tests.py (Diff revision 29)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  8. reviewboard/extensions/tests.py (Diff revision 29)
     
     
    Col: 9
     E303 too many blank lines (2)
    
  9. reviewboard/webapi/base.py (Diff revision 29)
     
     
    Col: 21
     E127 continuation line over-indented for visual indent
    
  10. 
      
AH
reviewbot
  1. 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
    
    
  2. reviewboard/extensions/hooks.py (Diff revision 30)
     
     
    Col: 80
     E501 line too long (86 > 79 characters)
    
  3. reviewboard/extensions/hooks.py (Diff revision 30)
     
     
    Col: 80
     E501 line too long (98 > 79 characters)
    
  4. reviewboard/webapi/base.py (Diff revision 30)
     
     
    Col: 21
     E127 continuation line over-indented for visual indent
    
  5. 
      
chipx86
  1. 
      
  2. Too many spaces after "An".

  3. The PRIVATE, READ_ONLY, etc. don't mean much at this point when reading through the docs. I'd try rethinking how you could write this to better describe the various states. Maybe say something like "The access states can be one of:" with a list mapping the :py:data: of each access state name to a description.