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

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"

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

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

Needs a docstring.

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.

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 …

'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: 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: 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: 1 E303 too many blank lines (3)

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

Can we call this register_extra_data_access_callback ?

Likewise unregister_extra_data_access_callback

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)

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

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.

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

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.

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

callbable -> callable

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: 21 E127 continuation line over-indented for visual indent