Implement review request action registration.

Review Request #7661 — Created Sept. 26, 2015 and submitted

adriano
Review Board
master
reviewboard, students

This is an architectural change for Review Board 2.6 that affects
extensions. Currently, there are some default review request actions
(like "Close -> Submitted" or "Download Diff" or "Ship It!") that have
been hardcoded into the template. Extensions can provide custom actions
by passing dictionaries into a variety of ActionHooks.

This change encapsulates this logic into a BaseReviewRequestAction
class. Instead of passing ActionHook-style dictionaries into the hooks,
extensions can now pass in BaseReviewRequestAction instances. Hardcoded
actions have been ripped out from the template and retrofitted into a
cleaner action registration system. Other new features include:

- Up to two levels of action nesting via BaseReviewRequestMenuActions.
- A should_render() method that subclasses of BaseReviewRequestAction
can override in order to implement complex action rendering behaviour.
- A register_actions() method that can be used to add a custom action to
any of the default menu actions.
- An unregister_actions() method that can be used to remove any of the
default actions.
- A ReviewRequestActionHookModel for providing action callbacks in JS.

- Ran and passed the relevant reviewboard unit tests.
- Manually verified that all default actions display correctly in
  different scenarios.
    1. Test in all three types of pages ("Reviews", "File", "Diff").
    2. Test in normal as well as local sites.
    3. Test DownloadDiffAction with the slider (for interdiffs).
    4. Test as an authenticated user and as a guest.
    5. Test when the review request is pending review and is submitted.
- Added my own extension that provides its own actions in different ways
  and manually verified that each action displayed correctly in
  different scenarios.
    1. Test adding actions and menu actions via action-style dicts.
    2. Test adding actions and menu actions via action instances.
    3. Test adding menu actions that contain their own menu actions.
    4. Test that a menu action can override its children from rendering.
    5. Test unregistering a default action.
    6. Test unregistering a child action of a default menu action.
    7. Test registering a new child action on a default menu action.
- Added unit tests that cover the above functionality, which all pass.
- Added my own JS extension that provides its own callback and manually
  verified that the callback is correctly triggered when its corresponding
  action is clicked.
- Built the Sphinx docs and manually verified that my updates rendered
  correctly.
Description From Last Updated

These shouldnt be on the class. They should be module-levels. See my other comment about the pages/forms stuff.

brenniebrennie

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

reviewbotreviewbot

Ideally, the should_render function should be a member funciton. This way, the people using this code can override the function ...

brenniebrennie

These should probably all be class attributes.

brenniebrennie

Re: my earlier comment, this would be class EditReviewAction(ReviewRequestAction): """...""" def should_render(self, request): return request.user.is_authenticated()

brenniebrennie

You should look at reviewboard/accounts/pages.py and reviewboard/accounts/forms/pages.py to see how we register account pages and forms. We're going to want ...

brenniebrennie

Style nit: blank line between these.

brenniebrennie

BLank line between these.

brenniebrennie

This file should start with from __future__ import unicode_literals

daviddavid

Docstrings should be in the imperative mood ("Return" vs "Returns")

daviddavid

Imperative mood ("Populates" -> "Populate")

daviddavid

Returns -> Return

daviddavid

Registers -> Register

daviddavid

This file should start with the unicode_literals import.

daviddavid

This will need to be able to change based on what revision of the diff a user is looking at.

daviddavid

Place the comment above this and prefix it with a colon, e.g.: #: A mapping of action IDs to action ...

brenniebrennie

Likewise here. However, this doesn't need a #: comment.

brenniebrennie

Can you give this a list of arguments? e.g. """.... Args: arg (arg_type): Brief description of arg. """ This comment ...

brenniebrennie

:py:exc:KeyError

brenniebrennie

Blank line between these.

brenniebrennie

We prefer %-expressions for formatting over ''.format.

brenniebrennie

You should have a constant on the class, e.g. class DownloadDiffAction(BaseReviewRequestAction): # ... #: Brief comment descrbing what this does. ...

brenniebrennie

Blank line between statement and block.

brenniebrennie

This needs a better variable name. Perhaps, content?

brenniebrennie

:py:class:`BaseReviewRequestAction`

brenniebrennie

This should use reST clode blocks. E.g., For example: .. code-block:: python class UsedOnceAction(BaseReviewRequestAction): action_id = 'once' label = 'This ...

brenniebrennie

Blank line between these.

brenniebrennie

This should go above args, e.g.: """Return whether or not this action should be rendered. The default implementation is to ...

brenniebrennie

Blank line between these.

brenniebrennie

This should go above Args

brenniebrennie

This should just be KeyError. The :py:exc:Exception stuff is for inline in strings, not in things that the docs expect ...

brenniebrennie

Needs a docstring. Likewise for all members of this class.

brenniebrennie

Needs a docstring. Also, blank line between this and action_id. Likewise for encapsulated classes.

brenniebrennie

This should be above args.

brenniebrennie

No blank line between the docstring and the first line of code for functions.

daviddavid

No blank line between the docstring and the first line of code for functions.

daviddavid

Because you're manipulating this in a method, it probably shouldn't be data on the class. Can you create an __init__ ...

daviddavid

No blank line between the docstring and the first line of code for functions.

daviddavid

No blank line between the docstring and the first line of code for functions.

daviddavid

No blank line between the docstring and the first line of code for functions.

daviddavid

No blank line between the docstring and the first line of code for functions.

daviddavid

This comment confuses me.

daviddavid

Can we call this review_request? We don't abbreviate to "rev" this way.

daviddavid

You should be able to just use .display_id here.

daviddavid

No blank line between docstring and code in functions.

daviddavid

These two lines could be combined (you only use actions once)

daviddavid

Hmm. I'm not sure how I feel about passing in a list and having the render() method append to it. ...

daviddavid

Let's put the whole if/endif thing on the same line.

daviddavid

Add another blank line after this.

daviddavid

I'm not sure why all of these are class methods. Can you clarify for me?

daviddavid

Can we just pull these out into top-level classes? There's no real reason to hide them inside their parent menus, ...

daviddavid

"children_actions" is kind of redundant. How about either "children" or "child_actions"?

daviddavid

Same with these (pull them out into top-level classes).

daviddavid

This is probably not a terribly reliable way to detect this. It might be a lot better to pass the ...

daviddavid

We should maybe also pass the request into this, and introspect the url/view name, instead of trying to reverse engineer ...

daviddavid

Can we wrap this somehow? Maybe split the line after the href="..."?

daviddavid

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Yes, it should be. __all__ defines which symbols get exported from the file (if there's no __all__, everything is exported).

daviddavid

_() doesn't work this way. The l10n scanner looks at the source code and extracts all string literals which are ...

daviddavid

Same here.

daviddavid

I would wrap this differently: super(DiffViewerActionHook, self).__init__( extension, apply_to, actions, *args, **kwargs)

daviddavid

Add a blank line after this.

daviddavid

Because we're intending to return a bool (and not a Repository), we should make this a truth test: return review_request.repository ...

daviddavid

Instead of using 'P' as a literal, you can compare this against ReviewRequest.PENDING_REVIEW.

daviddavid

'_' imported but unused

reviewbotreviewbot

Mind fixing the spelling here while you're editing this file?

daviddavid

It doesn't really matter, but I think it would be nicer to have this set after we register all the ...

daviddavid

Instead of just casting to bool, I think we should explicitly test review_request.repository is not None.

daviddavid

This can be rewritten using logging.exception, which we're slowly transitioning to: logging.exception('Error rendering top-level action: %s', top_level_action.action_id)

daviddavid

Same here.

daviddavid

URL.

brenniebrennie

JavaScript, ``'#'``

brenniebrennie

``javascript:`` URL

brenniebrennie

2.6 is feature frozen. Can you change this to 2.7?

brenniebrennie

The right side can be reformatted to fit more on the first line.

brenniebrennie

Again, 2.7.

brenniebrennie

2.7

brenniebrennie

The from should be lined up with code.

brenniebrennie

Please format this as: from djblets.extensions.hooks import (AppliesToURLMixin, DataGridColumnsHook, ExtensionHook, ExtensionHookPoint, SignalHook, TemplateHook, URLHook)

brenniebrennie

Format this as: super(BaseReviewRequestActionHook, self).__init__( extension, apply_to=apply_to, *args, **kwargs)

brenniebrennie

Needs Args and Returns, e.g. """Single line summary. Multi-line description. Args: actions (list): A brief description of what actions is. ...

brenniebrennie

Blank line between these.

brenniebrennie

So registered_actions is a temporary that is only used in this function, but we're creating a iterator out of it ...

brenniebrennie

Needs Args and Returns.

brenniebrennie

This should be a class member, or a global member. I'm not sure. But it definitely shouldn't be in this ...

brenniebrennie

Needs a better docstring to explain why this is necessary.

brenniebrennie

Should be done in __init__.

brenniebrennie

Please format as: super(ReviewRequestActionHook, self).__init__( extension, apply_to, actions, *args, **kwargs)

brenniebrennie

Likewise, I don't think this should be in the def.

brenniebrennie

Format as: return TempMenuAction([ super(ReviewRequestDropdownActionHook, self)._to_action_instance( child_action_dict) for child_action_dict in action_dict['items'] ])

brenniebrennie

Alphabetize these imports.

brenniebrennie

Put this on the previous line.

brenniebrennie

Put this on the previous line.

brenniebrennie

Put this on the previous line.

brenniebrennie

Put this on the previous line.

brenniebrennie

Put this on the previous line.

brenniebrennie

Put this on the previous line.

brenniebrennie

Needs a docstring.

brenniebrennie

Undo this.

brenniebrennie

Undo this.

brenniebrennie

Undo this.

brenniebrennie

The text should line up with the n in note.

brenniebrennie

Can you format this as: parent (BaseReviewRequestMenuAction): The parent action instance of this action instance (optional).

brenniebrennie

Text should align with n in note.

brenniebrennie

Put on the previous line.

brenniebrennie

Should be a complete sentence.

brenniebrennie

Docstring should mention that this is a generator.

brenniebrennie

""" Yields: BaseReviewREquestActions: All top-level registered review request action instances. """

brenniebrennie

Format as: """ KeyError: ... """

brenniebrennie

Should be complete sentence.

brenniebrennie

"Determines" over "Tracks"

brenniebrennie

Put this on the previous line please.

brenniebrennie

"otherwise" instead of "else".

brenniebrennie

Needs Returns.

brenniebrennie

I'd rather not use a table for this (and require the shorthand for the actions). A bullet point list is ...

chipx86chipx86

The description shouldn't be on the same line as the version number. Look around the docs codebase and you'll find ...

chipx86chipx86

Two blank lines between sections. Same below.

chipx86chipx86

What's this for? Ideally, all named lists of URLs should be defined in exactly one place, reviewboard.urls, so depending on ...

chipx86chipx86

This should spend some time describing what actions are and roughly how they work, like the old docs did.

chipx86chipx86

I'd prefer not to align like this. Instead: * ``id``: The ID of the action (optional). * ``label``: ... (Make ...

chipx86chipx86

Blank line between these.

chipx86chipx86

This needs a docstring.

chipx86chipx86

This definitely need more docs. I don't have a good sense of what this does.

chipx86chipx86

This is assuming some things about the data passed in, and if the keys are missing, the caller's going to ...

chipx86chipx86

This looks as if the self.label... is a third parameter. I'd instead do: self.action_id = action_dict.get( 'id', ...)

chipx86chipx86

This should also be fleshed out more.

chipx86chipx86

Same stuff as above.

chipx86chipx86

These are best referenced using :guilabel:.

chipx86chipx86

Maybe list these, so a caller doesn't have to look through the class hierarchy and check parent classes.

chipx86chipx86

Needs a docstring.

chipx86chipx86

Instead of setting these, just pass them inline where needed.

chipx86chipx86

This is kind of named awkwardly. From the name, I have no real sense of what it's doing, and the ...

chipx86chipx86

:py:class: for dict.

chipx86chipx86

BaseReviewRequestAction

chipx86chipx86

I notice we're reversing here, and then reversing again later. What's the reason for that?

chipx86chipx86

What other values can this be? It might want to sanity-check that everything provided is an expected type of value, ...

chipx86chipx86

Blank line between these.

chipx86chipx86

A function beginning with to_... generally means that the owning class would be converted to a thing. Let's go with ...

chipx86chipx86

Let's flesh this out a bit, mention that this means it'll only show up on the review request page containing ...

chipx86chipx86

Needs a docstring.

chipx86chipx86

Same above. We should list the possible types.

chipx86chipx86

Same as above with the formatting.

chipx86chipx86

This should go in an Example section, like: """ ... Example: .. code-block:: python actions = [{ ... }]

chipx86chipx86

Same naming thoughts as above. Also, needs docstrings.

chipx86chipx86

Same comments as above.

chipx86chipx86

This is one of the older unit test suites in the codebase. Let's go ahead and move all action unit ...

chipx86chipx86

I know the old unit test names didn't do this, but ideally, all new unit test names should reference the ...

chipx86chipx86

These are nice cleanups, but really should be broken out into its own change, to help keep this change slimmer. ...

chipx86chipx86

This should go in an "Example:" section, like: """ ... Example: .. code-block:: python: class .... """ We've been placing ...

chipx86chipx86

These should be documented inline where defined on the class. "Attributes:" is more useful when documenting things only set in ...

chipx86chipx86

Private attributes shouldn't be documented. Same below.

chipx86chipx86

If a subclass is meant to override this, it should not be a private function.

chipx86chipx86

Should be in a "Note:" section instead.

chipx86chipx86

Should be indented one level.

chipx86chipx86

Same here.

chipx86chipx86

You can combine these to be optimistic: try: del _all_actions[...] except KeyError: raise KeyError(...)

chipx86chipx86

Not worth repeating these. Same below.

chipx86chipx86

Needs a docstring.

chipx86chipx86

Blank line between these, generally, but this might be better as: try: parent = _all_actions[parent_id] except KeyError: raise KeyError(...)

chipx86chipx86

Same as above.

chipx86chipx86

Let's flesh this out just a bit, describing the resulting state. May also need a warning that this will impact ...

chipx86chipx86

Blank line before blocks.

chipx86chipx86

Blank line between these. Though, you might want to just combine the statements.

chipx86chipx86

Let's have the third condition on its own line.

chipx86chipx86

So I have a concern here, and I want to follow up on how this works. We're registering instances, right? ...

chipx86chipx86

Blank line between these.

chipx86chipx86

Here too. Though, you may want to just combine the statements.

chipx86chipx86

Parens around the comparison.

chipx86chipx86

Description on the next line.

chipx86chipx86

This might need to provide an example.

chipx86chipx86

This should be documented.

chipx86chipx86

If we're changing the names, let's get rid of -link and replace it with -action.

chipx86chipx86

No space after function.

chipx86chipx86

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

reviewbotreviewbot

Col: 17 W503 line break before binary operator

reviewbotreviewbot

'register_actions' imported but unused

reviewbotreviewbot

'unregister_actions' imported but unused

reviewbotreviewbot

A form we're moving to, which reads and maintains better, is: ``id`` (optional): The ID of... ``label``: ...

chipx86chipx86

Only two backticks are needed.

chipx86chipx86

Any place we're referencing some key, we should use double backticks instead of making it bold.

chipx86chipx86

Not sure that we should document registering without a hook in an extension, since that means more that the extension ...

chipx86chipx86

We should document that this works on all review request pages.

chipx86chipx86

This is probably not something we want to explicitly document here. Instead, we should somehow save and restore the actions ...

chipx86chipx86

Like with the docs, let's do: ``id`` (optional): ... ``label``: ...

chipx86chipx86

Can you add ", optional" to the end of this, inside the parens? Same with any other arguments you have ...

chipx86chipx86

Technically, this is a tuple. Same below.

chipx86chipx86

The type should be "callable".

chipx86chipx86

It's not generally good for classes to override functions like this. Let's define a proper method instead. Same below.

chipx86chipx86

I'm actually not sure, what is #. ?

chipx86chipx86

Should have ", optional".

chipx86chipx86

"Raises" should go after "Returns". Same below.

chipx86chipx86

We should use :py:class: here. Same below.

chipx86chipx86

No indentation for the description here. Same below.

chipx86chipx86

Maybe add a comment saying why we do this in reverse order.

chipx86chipx86

Can return reversed(registered_actions) instead.

chipx86chipx86

Should have :py:class on the class names in docstrings. Same elsewhere.

chipx86chipx86

Should have ", optional"

chipx86chipx86

Same as above with the formatting.

chipx86chipx86

No period on unit test docstrings. Same below.

chipx86chipx86

Let's specify the base class name.

chipx86chipx86

Let's specify the class name.

chipx86chipx86

I think "height" might actually be a bit too confusing. "Depth" worked better. "Height" usually means the dimensional height of ...

chipx86chipx86

Maybe "as HTML." ?

chipx86chipx86

Should have ", optional". Then there's no need for "(optional)" at the end.

chipx86chipx86

Should wrap this in a try:, in case there's a failure, and log in except:. Then we can pop in ...

chipx86chipx86

No need for the inner parens.

chipx86chipx86

% should go on the next line. Not sure RuntimeError is right here. Is this something that should never happen, ...

chipx86chipx86

No need for the inner parens.

chipx86chipx86

", optional"

chipx86chipx86

"as HTML." Same with any others.

chipx86chipx86

", optional"

chipx86chipx86

This comment isn't about this line per se, but we have this nifty new Registry thing that Barret put together ...

chipx86chipx86

", optional"

chipx86chipx86

No need for inner parens.

chipx86chipx86

Blank line between these.

chipx86chipx86

As mentioned earlier in the review, this is kind of dangerous for extensions, because they can stomp all over each ...

chipx86chipx86

We can now use @register.simple_tag(takes_context=True). @basictag is newly-deprecated. Same below.

chipx86chipx86

Blank line between these. Same with ones below.

chipx86chipx86

We should probably call this in tearDown().

chipx86chipx86

We have a new way of handling the documentation of model attributes. See https://www.reviewboard.org/docs/codebase/dev/docs/writing-codebase-docs/#documenting-models

chipx86chipx86

Instead of a list (since this is "only" one thing), maybe call this main_review_request_url_name?

chipx86chipx86
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/builtin_actions.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/reviews/review_request_actions_primary.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/builtin_actions.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/reviews/review_request_actions_primary.html
    
    
  2. reviewboard/reviews/actions.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
    1. So...the line limit is 79, not 80?

    2. According to PEP8, that is correct: http://legacy.python.org/dev/peps/pep-0008/#maximum-line-length

    3. Fair enough, thanks.

  3. 
      
mike_conley
  1. 
      
  2. reviewboard/reviews/actions.py (Diff revision 1)
     
     
     
     
     

    From what I can tell, the only difference between primary and secondary actions was their positions in the list (where primary were always put after secondary, so they're closer to the right).

    1. Thanks for the clarification. From what you said, it seems that I don't really need to worry about this. Extensions should probably be able to reorder actions as desired, instead of grouping them into primary or secondary.

  3. reviewboard/reviews/actions.py (Diff revision 1)
     
     

    So I see what you're doing here with these class methods, and it makes sense, but I don't think it's consistent with the way the rest of RB's extension framework works - specifically, the Fields API, which I believe this new Actions API is supposed to expose.

    Here are some Fields defined in the MozReview extension, for example: https://hg.mozilla.org/hgcustom/version-control-tools/file/d1e664f1dc8d/pylib/mozreview/mozreview/fields.py

    And here are the built-in fields being manipulated: https://hg.mozilla.org/hgcustom/version-control-tools/file/835dbd9444db/pylib/mozreview/mozreview/extension.py#l140

    So I suspect what we want to do is expose some functions in actions.py that can be used to manipulate the current actions, and an equivalent to ReviewRequestFieldsHook (but for Actions) that makes it easy (and backwards compatible for ReviewRequestActionsHook... I'll just call it ReviewRequestActionsHook2 for now to differentiate, but that shouldn't be its name) to inject new Actions.

    So the final result would be that, as an extension developer, I subclass something like a ReviewRequestBaseAction, and override something like a "should_render" method for saying whether or not the Action should apply under the current conditions.

    And then I would instantiate the new ReviewRequestActionsHook2, passing in the subclass, along with the URL name that the hook should apply to.

    Does that make sense?

    1. Thanks, that makes a lot of sense. I originally had copied over the same structure used in the fields.py file, but as an experiment I wanted to see what would happen if I "brought everything one level down". Surprisingly, it sort of worked.

      I understand that consistency is important (my experiment will be easy enough to revert), but I am a bit curious as to why the code is slightly more complicated than it perhaps needs to be. If we create an EditReviewAction class that subclasses some BaseReviewRequestAction (instead of instantiating an edit_review_action object from a ReviewRequestAction class), then does this mean that we could instantiate multiple objects from a single EditReviewAction class? To me, it seems that there is a one-to-one correspondence between an action class and an action instance (the lack of instance-level attributes confuses me).

  4. 
      
brennie
  1. 
      
  2. reviewboard/reviews/actions.py (Diff revision 1)
     
     
     

    These shouldnt be on the class. They should be module-levels. See my other comment about the pages/forms stuff.

  3. reviewboard/reviews/actions.py (Diff revision 1)
     
     

    Ideally, the should_render function should be a member funciton. This way, the people using this code can override the function (which by default should probably return True) to do complicated stuff with, e.g., the request.

  4. reviewboard/reviews/actions.py (Diff revision 1)
     
     
     
     

    These should probably all be class attributes.

  5. reviewboard/reviews/builtin_actions.py (Diff revision 1)
     
     
     
     
     
     
     

    Re: my earlier comment, this would be

    class EditReviewAction(ReviewRequestAction):
        """..."""
    
        def should_render(self, request):
            return request.user.is_authenticated()
    
    1. Thanks. As I mentioned in a reply to Mike, I originally had copied over the same structure used in the fields.py file, but as an experiment I wanted to see what would happen if I "brought everything one level down". Surprisingly, it sort of worked. At any rate, I'm going to go ahead and revert my experiment.

      I am a little bit confused as to when we should ever put things at the instance level (instead of the module or class level). If we make an EditReviewAction class that subclasses some BaseReviewRequestAction (instead of instantiating an edit_review_action object from a ReviewRequestAction class), then does this mean that we could instantiate multiple objects from a single EditReviewAction class? To me, it seems that there is a one-to-one correspondence between an action class and an action instance (the lack of instance-level attributes confuses me).

      In particular, should this should_render method actually be a class method (and can it access the full context, since this is needed by the Download Diff action)? Something like:

      class EditReviewAction(BaseReviewRequestAction):
          """..."""
      
          @classmethod
          def should_render(cls, context):
              return context['request'].user.is_authenticated()
      
    2. You may want to ping Christian about whether or not these should be @classmethods or instance methods.

  6. reviewboard/reviews/builtin_actions.py (Diff revision 1)
     
     
     
     
     
     

    You should look at reviewboard/accounts/pages.py and reviewboard/accounts/forms/pages.py to see how we register account pages and forms. We're going to want a similar system to that that. Feel free to ping me in slack if you have questions about it.

    1. Please don't mark issues as fixed that haven't been; drop them and leave a reply instead.

    2. Please disregard that last comment :)

      I saw the same code and left that before I finished revieweing. Sorry!

  7. Style nit: blank line between these.

  8. BLank line between these.

  9. 
      
AD
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/builtin_actions.py
    
    Ignored Files:
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_detail.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/builtin_actions.py
    
    Ignored Files:
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_detail.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/reviews/actions.py (Diff revision 2)
     
     

    This file should start with from __future__ import unicode_literals

  3. reviewboard/reviews/actions.py (Diff revision 2)
     
     

    Docstrings should be in the imperative mood ("Return" vs "Returns")

    1. Thanks, I didn't know that. Many of the docstrings in this actions.py file were taken directly from the fields.py file, which doesn't quite use the imperative mood. Should I update the docstrings in the fields.py file as well to be consistent?

      Also, I added a new for_review_request_action method in templatetags/reviewtags.py whose docstring starts with "Loops through all review request actions." Like the fields.py file, the docstrings in templatetags/reviewtags.py also don't use the imperative mood. Should these docstrings be updated as well?

  4. reviewboard/reviews/actions.py (Diff revision 2)
     
     

    Imperative mood ("Populates" -> "Populate")

  5. reviewboard/reviews/actions.py (Diff revision 2)
     
     

    Returns -> Return

  6. reviewboard/reviews/actions.py (Diff revision 2)
     
     

    Registers -> Register

  7. reviewboard/reviews/builtin_actions.py (Diff revision 2)
     
     

    This file should start with the unicode_literals import.

  8. reviewboard/reviews/builtin_actions.py (Diff revision 2)
     
     

    This will need to be able to change based on what revision of the diff a user is looking at.

  9. 
      
AD
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/builtin_actions.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_detail.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/builtin_actions.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_detail.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/reviews/builtin_actions.py (Diff revision 3)
     
     
     

    You should have a constant on the class, e.g.

    class DownloadDiffAction(BaseReviewRequestAction):
        # ...
    
        #: Brief comment descrbing what this does.
        URL_RE = re.compile(r'...')
    
        # ...
    
  3. reviewboard/reviews/builtin_actions.py (Diff revision 3)
     
     
     

    Blank line between statement and block.

  4. This needs a better variable name. Perhaps, content?

  5. 
      
brennie
  1. 
      
  2. reviewboard/reviews/actions.py (Diff revision 3)
     
     

    Place the comment above this and prefix it with a colon, e.g.:

    #: A mapping of action IDs to action classes.
    _action_classes = SortedDict()
    

    That way, this member will end up in auto-generated documentation.

  3. reviewboard/reviews/actions.py (Diff revision 3)
     
     

    Likewise here. However, this doesn't need a #: comment.

  4. reviewboard/reviews/actions.py (Diff revision 3)
     
     
     
     
     
     

    Can you give this a list of arguments?

    e.g.

    """....
    
    
    Args:
        arg (arg_type):
            Brief description of arg.
    """
    

    This comment applies to all functions that take arguments.
    That way they will show up in autogenerated documentation.

  5. reviewboard/reviews/actions.py (Diff revision 3)
     
     

    :py:exc:KeyError

  6. reviewboard/reviews/actions.py (Diff revision 3)
     
     
     

    Blank line between these.

  7. reviewboard/reviews/actions.py (Diff revision 3)
     
     
     

    We prefer %-expressions for formatting over ''.format.

  8. 
      
AD
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/builtin_actions.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_detail.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/builtin_actions.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_detail.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
AD
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/builtin_actions.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/templates/reviews/ui/base.html
        reviewboard/templates/reviews/review_detail.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/templates/reviews/action.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
        reviewboard/templates/reviews/menu_action.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/builtin_actions.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/templates/reviews/ui/base.html
        reviewboard/templates/reviews/review_detail.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/templates/reviews/action.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
        reviewboard/templates/reviews/menu_action.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    
    
  2. 
      
brennie
  1. This is shaping up nicely :)

    There are quite a few missing docstrings and I didn't flag all instane of this. In general, all classes should have docstrings. All methods (except things that are being overridden, unless they have a special use case) should be documented as well.

    As far as formatting goes, classes should be formatted as follows (note the blank line between the docstring and the attribute):

    class ClassName(object):
        """Single line summary.
    
        Multi-line description.
        """
    
        first_member = 1
    

    Also, things like side effects and important notes should be in the multi-line description of the docstring, not under a separate section. The args/return sections are pulled out by sphinx into build docs, but those other sections won't be.

  2. reviewboard/reviews/actions.py (Diff revision 5)
     
     

    :py:class:`BaseReviewRequestAction`
    
  3. reviewboard/reviews/actions.py (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    This should use reST clode blocks.

    E.g.,

    For example:
    
    .. code-block:: python
       class UsedOnceAction(BaseReviewRequestAction):
           action_id = 'once'
           label = 'This is used once.'
    
       class UsedMultipleAction(BaseReviewRequestAction):
           def __init__(self, action_id, label):
               self.action_id = 'repeat_' + action_id
               self.label = label
    

    This really shouldn't be REPL output either

  4. reviewboard/reviews/actions.py (Diff revision 5)
     
     
     

    Blank line between these.

  5. reviewboard/reviews/actions.py (Diff revision 5)
     
     
     
     

    This should go above args, e.g.:

    """Return whether or not this action should be rendered.
    
    The default implementation is to always render the action.
    
    Args:
    ...
    
  6. reviewboard/reviews/actions.py (Diff revision 5)
     
     
     

    Blank line between these.

  7. reviewboard/reviews/actions.py (Diff revision 5)
     
     
     

    This should go above Args

  8. reviewboard/reviews/actions.py (Diff revision 5)
     
     
     

    This should just be KeyError.

    The :py:exc:Exception stuff is for inline in strings, not in things that the docs expect to be types.

  9. reviewboard/reviews/builtin_actions.py (Diff revision 5)
     
     

    Needs a docstring. Likewise for all members of this class.

  10. reviewboard/reviews/builtin_actions.py (Diff revision 5)
     
     

    Needs a docstring. Also, blank line between this and action_id. Likewise for encapsulated classes.

  11. reviewboard/reviews/builtin_actions.py (Diff revision 5)
     
     
     

    This should be above args.

  12. 
      
AD
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/reviews/default_actions.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/templates/reviews/ui/base.html
        reviewboard/templates/reviews/review_detail.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/templates/reviews/action.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
        reviewboard/templates/reviews/menu_action.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/reviews/default_actions.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/templates/reviews/ui/base.html
        reviewboard/templates/reviews/review_detail.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/templates/reviews/action.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
        reviewboard/templates/reviews/menu_action.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    
    
  2. 
      
AD
david
  1. This looks like a great start!

  2. reviewboard/reviews/actions.py (Diff revision 6)
     
     

    No blank line between the docstring and the first line of code for functions.

    1. I did have it like that originally, but Barret recommended putting a newline there (https://reviews.reviewboard.org/r/7661/#review22277).

    2. So we have a blank line between the docstring and the first thing in a class, but not a function. The distinction here is that members/methods in a class always have a blank line separating them (and the docstring is a member), but lines of code in a function don't.

  3. reviewboard/reviews/actions.py (Diff revision 6)
     
     

    No blank line between the docstring and the first line of code for functions.

  4. reviewboard/reviews/actions.py (Diff revision 6)
     
     

    Because you're manipulating this in a method, it probably shouldn't be data on the class. Can you create an __init__ and assign this there?

  5. reviewboard/reviews/actions.py (Diff revision 6)
     
     

    No blank line between the docstring and the first line of code for functions.

  6. reviewboard/reviews/actions.py (Diff revision 6)
     
     

    No blank line between the docstring and the first line of code for functions.

  7. reviewboard/reviews/actions.py (Diff revision 6)
     
     

    No blank line between the docstring and the first line of code for functions.

  8. reviewboard/reviews/default_actions.py (Diff revision 6)
     
     

    No blank line between the docstring and the first line of code for functions.

  9. reviewboard/reviews/default_actions.py (Diff revision 6)
     
     

    This comment confuses me.

    1. This comment is intended to explain why I am using context.get('has_diffs', True) instead of context['has_diffs']. A review request can be viewed from three different tabs: "Reviews", "File", and "Diff" (the third "File" tab only appears when viewing file attachments). In the "Reviews" and "File" tabs, there is a has_diffs key that can be looked up in the context dictionary. Its corresponding value in the dictionary is set to True iff the review request has at least one diff.

      In the "Diff" tab on the other hand, the has_diffs key does not exist in the context dictionary. It makes sense not to include this redundant piece of information; after all, the only way we could have accessed the "Diff" tab in the first place is if the corresponding review request had a diff already.

      Perhaps a better comment would be something like:

      # If has_diffs does not exist, then we must be in the Diff tab.
      # So the corresponding review request must already have a diff.
      
  10. reviewboard/reviews/default_actions.py (Diff revision 6)
     
     

    Can we call this review_request? We don't abbreviate to "rev" this way.

  11. reviewboard/reviews/default_actions.py (Diff revision 6)
     
     

    You should be able to just use .display_id here.

  12. No blank line between docstring and code in functions.

  13. reviewboard/reviews/templatetags/reviewtags.py (Diff revision 6)
     
     
     
     

    These two lines could be combined (you only use actions once)

  14. Hmm. I'm not sure how I feel about passing in a list and having the render() method append to it. How about we let render() return a string, and then we build the result using a list comprehension?

    return ''.join([
        action.render(context)
        for action in get_review_request_actions()
    ])
    

    This way the data flow in render() is entirely one-way.

  15. reviewboard/templates/reviews/action.html (Diff revision 6)
     
     
     

    Let's put the whole if/endif thing on the same line.

  16. 
      
AD
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/reviews/default_actions.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/templates/reviews/ui/base.html
        reviewboard/templates/reviews/review_detail.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/templates/reviews/action.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
        reviewboard/templates/reviews/menu_action.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/reviews/default_actions.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/templates/reviews/ui/base.html
        reviewboard/templates/reviews/review_detail.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/templates/reviews/action.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
        reviewboard/templates/reviews/menu_action.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    
    
  2. 
      
AD
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/extensions/tests.py
        reviewboard/extensions/hooks.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/default_actions.py
        reviewboard/extensions/templatetags/rb_extensions.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/templates/reviews/ui/base.html
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/extensions/action_dropdown.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/templates/reviews/action.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
        reviewboard/templates/reviews/menu_action.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/extensions/tests.py
        reviewboard/extensions/hooks.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/default_actions.py
        reviewboard/extensions/templatetags/rb_extensions.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/templates/reviews/ui/base.html
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/extensions/action_dropdown.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/templates/reviews/action.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
        reviewboard/templates/reviews/menu_action.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    
    
  2. 
      
AD
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/extensions/tests.py
        reviewboard/extensions/hooks.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/default_actions.py
        reviewboard/extensions/templatetags/rb_extensions.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/templates/reviews/ui/base.html
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/extensions/action_dropdown.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/templates/reviews/action.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
        reviewboard/templates/reviews/menu_action.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/extensions/tests.py
        reviewboard/extensions/hooks.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/default_actions.py
        reviewboard/extensions/templatetags/rb_extensions.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/templates/reviews/ui/base.html
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/extensions/action_dropdown.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/templates/reviews/action.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
        reviewboard/templates/reviews/menu_action.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    
    
  2. 
      
AD
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/extensions/tests.py
        reviewboard/extensions/hooks.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/default_actions.py
        reviewboard/extensions/templatetags/rb_extensions.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/templates/reviews/ui/base.html
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/extensions/action_dropdown.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/templates/reviews/action.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
        reviewboard/templates/reviews/menu_action.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/extensions/tests.py
        reviewboard/extensions/hooks.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/default_actions.py
        reviewboard/extensions/templatetags/rb_extensions.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/templates/reviews/ui/base.html
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/extensions/action_dropdown.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/templates/reviews/action.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
        reviewboard/templates/reviews/menu_action.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/extensions/hooks.py (Diff revision 10)
     
     

    Add another blank line after this.

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

    I'm not sure why all of these are class methods. Can you clarify for me?

    1. I was switching a lot between different types of methods actually. At first, they were static methods because they never used instance attributes like actions. Then I changed them to class methods because for example _register_and_get() calls _to_action_instance(), and subclasses might want to override these methods with their own implementations.

      I suppose that we could extend this argument and say that extensions might want different instances of the same subclass to override these methods in different ways. For example:

      class Foo(BaseReviewRequestActionHook):
          def __init__(self, extension, actions=[], is_awesome=True, *args, **kwargs):
              super(Foo, self).__init__(extension, actions, *args, **kwargs)
              self.is_awesome = is_awesome
      
          def action_should_render(self, context):
              if not self.is_awesome:
                  return False
              return super(Foo, self).action_should_render(self, context)
      

      So in the interest of being as flexible as possible, I guess I should just change them all to be simple instance methods. (Also, I forgot to put an underscore before action_should_render() - it should be a "protected" method as well.)

    2. Instance methods sound good to me. In Python, static methods are used only occasionally and class methods are used almost never.

  4. reviewboard/reviews/default_actions.py (Diff revision 10)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Can we just pull these out into top-level classes? There's no real reason to hide them inside their parent menus, and it makes it harder to read.

  5. reviewboard/reviews/default_actions.py (Diff revision 10)
     
     

    "children_actions" is kind of redundant. How about either "children" or "child_actions"?

  6. reviewboard/reviews/default_actions.py (Diff revision 10)
     
     

    Same with these (pull them out into top-level classes).

  7. reviewboard/reviews/default_actions.py (Diff revision 10)
     
     
     
     
     

    This is probably not a terribly reliable way to detect this. It might be a lot better to pass the review request into should_render.

    1. I'm not sure what you mean by this. Basically, the should_render(self, context) method gets called from only one place: render(self, context, action_key, template_name). If I change the function prototype to something like should_render(self, context, review_request), then how would I get the review request in the first place? The only way I know how to retrieve the current review request is by grabbing it from the context, so I would naively do something like:

      def render(self, context, action_key, template_name):
          content = ''
      
          if self.should_render(context, context['review_request']):
              context.push()
              context[action_key] = self
              content = render_to_string(template_name, context)
              context.pop()
      
          return content
      

      Is there a special call that I can make to retrieve the current review request from somewhere other than the context?


      Alternatively, perhaps you mean that it's fine to retrieve the review request from the context, but I shouldn't rely on using the has_diffs key. Instead, perhaps I should manually retrieve this from the review request by doing something like:

      def should_render(self, context):
          review_request = context['review_request']
          request = context['request']
          draft = review_request.get_draft(request.user)
          diffsets = review_request.get_diffsets()
      
          if (draft and draft.diffset_id) or len(diffsets) > 0:
              self.label = _('Update Diff')
          else:
              self.label = _('Upload Diff')
      
          return context['upload_diff_form']
      

      (I'm not terribly sure how I can check for an upload_diff_form without using the context.)

  8. reviewboard/reviews/default_actions.py (Diff revision 10)
     
     
     
     
     
     

    We should maybe also pass the request into this, and introspect the url/view name, instead of trying to reverse engineer it from the context contents.

  9. Can we wrap this somehow? Maybe split the line after the href="..."?

  10. 
      
AD
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/extensions/tests.py
        reviewboard/extensions/hooks.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/default_actions.py
        reviewboard/extensions/templatetags/rb_extensions.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/templates/reviews/ui/base.html
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/extensions/action_dropdown.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/templates/reviews/action.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
        reviewboard/templates/reviews/menu_action.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/extensions/tests.py
        reviewboard/extensions/hooks.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/default_actions.py
        reviewboard/extensions/templatetags/rb_extensions.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/templates/reviews/ui/base.html
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/extensions/action_dropdown.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/templates/reviews/action.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
        reviewboard/templates/reviews/menu_action.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    
    
  2. Col: 1
     E302 expected 2 blank lines, found 1
    
  3. 
      
AD
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/extensions/tests.py
        reviewboard/extensions/hooks.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/default_actions.py
        reviewboard/extensions/templatetags/rb_extensions.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/templates/reviews/ui/base.html
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/extensions/action_dropdown.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/templates/reviews/action.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
        reviewboard/templates/reviews/menu_action.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/extensions/tests.py
        reviewboard/extensions/hooks.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/default_actions.py
        reviewboard/extensions/templatetags/rb_extensions.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/templates/reviews/ui/base.html
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/extensions/action_dropdown.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/templates/reviews/action.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
        reviewboard/templates/reviews/menu_action.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    
    
  2. 
      
david
  1. This is starting to look very solid!

  2. reviewboard/extensions/hooks.py (Diff revision 12)
     
     

    Yes, it should be. __all__ defines which symbols get exported from the file (if there's no __all__, everything is exported).

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

    _() doesn't work this way. The l10n scanner looks at the source code and extracts all string literals which are wrapped with gettext. If you call this on a variable, the scanner won't pull it out.

    If actions want to localize, they should call ugettext_lazy (aka _()) on their labels themselves (which it looks like you're already doing).

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

    Same here.

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

    I would wrap this differently:

    super(DiffViewerActionHook, self).__init__(
        extension, apply_to, actions, *args, **kwargs)
    
  6. reviewboard/reviews/actions.py (Diff revision 12)
     
     

    Add a blank line after this.

  7. reviewboard/reviews/default_actions.py (Diff revision 12)
     
     

    Because we're intending to return a bool (and not a Repository), we should make this a truth test:

    return review_request.repository is not None
    
  8. reviewboard/reviews/default_actions.py (Diff revision 12)
     
     

    Instead of using 'P' as a literal, you can compare this against ReviewRequest.PENDING_REVIEW.

  9. 
      
AD
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/extensions/tests.py
        reviewboard/extensions/hooks.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/default_actions.py
        reviewboard/extensions/templatetags/rb_extensions.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/templates/reviews/ui/base.html
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/extensions/action_dropdown.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/templates/reviews/action.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
        reviewboard/templates/reviews/menu_action.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/extensions/tests.py
        reviewboard/extensions/hooks.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/default_actions.py
        reviewboard/extensions/templatetags/rb_extensions.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/templates/reviews/ui/base.html
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/extensions/action_dropdown.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/templates/reviews/action.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
        reviewboard/templates/reviews/menu_action.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    
    
  2. reviewboard/extensions/hooks.py (Diff revision 13)
     
     
     '_' imported but unused
    
  3. 
      
david
  1. 
      
  2. reviewboard/extensions/tests.py (Diff revision 13)
     
     

    Mind fixing the spelling here while you're editing this file?

  3. reviewboard/reviews/actions.py (Diff revision 13)
     
     

    It doesn't really matter, but I think it would be nicer to have this set after we register all the default actions.

    1. Hmm. I tried changing the order, but this results in infinite recursion since default_action.register() will call _populate_defaults() and enter the if statement forever. I suppose that we could instead use some kind of enum called _status that keeps track of three different states: UNPOPULATED, IS_POPULATING and POPULATED. Then we could have something like:

      def _populate_defaults():
          """Populate the default action instances."""
          global _status
      
          if _status != POPULATED:
              from reviewboard.reviews.default_actions import default_actions
      
              _status = IS_POPULATING
      
              for default_action in reversed(default_actions):
                  default_action.register()
      
              _status = POPULATED
      

      Was this what you had in mind?

  4. reviewboard/reviews/default_actions.py (Diff revision 13)
     
     

    Instead of just casting to bool, I think we should explicitly test review_request.repository is not None.

  5. This can be rewritten using logging.exception, which we're slowly transitioning to:

    logging.exception('Error rendering top-level action: %s',
                      top_level_action.action_id)
    
  6. Same here.

  7. 
      
AD
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/extensions/tests.py
        reviewboard/extensions/hooks.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/default_actions.py
        reviewboard/extensions/templatetags/rb_extensions.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/templates/reviews/ui/base.html
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/extensions/action_dropdown.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/templates/reviews/action.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
        reviewboard/templates/reviews/menu_action.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/extensions/tests.py
        reviewboard/extensions/hooks.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/default_actions.py
        reviewboard/extensions/templatetags/rb_extensions.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/templates/reviews/ui/base.html
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/extensions/action_dropdown.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/templates/reviews/action.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
        reviewboard/templates/reviews/menu_action.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    
    
  2. 
      
AD
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/extensions/tests.py
        reviewboard/extensions/hooks.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/default_actions.py
        reviewboard/staticbundles.py
        reviewboard/extensions/templatetags/rb_extensions.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        docs/manual/extending/coderef/index.rst
        reviewboard/templates/reviews/ui/base.html
        reviewboard/static/rb/js/extensions/models/reviewRequestActionHookModel.js
        docs/manual/extending/extensions/hooks/index.rst
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/extensions/action_dropdown.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        docs/manual/extending/extensions/hooks/action-hook.rst
        reviewboard/templates/reviews/action.html
        docs/manual/extending/extensions/hooks/action-hooks.rst
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/templates/reviews/menu_action.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/extensions/tests.py
        reviewboard/extensions/hooks.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/default_actions.py
        reviewboard/staticbundles.py
        reviewboard/extensions/templatetags/rb_extensions.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        docs/manual/extending/coderef/index.rst
        reviewboard/templates/reviews/ui/base.html
        reviewboard/static/rb/js/extensions/models/reviewRequestActionHookModel.js
        docs/manual/extending/extensions/hooks/index.rst
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/extensions/action_dropdown.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        docs/manual/extending/extensions/hooks/action-hook.rst
        reviewboard/templates/reviews/action.html
        docs/manual/extending/extensions/hooks/action-hooks.rst
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/templates/reviews/menu_action.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
brennie
  1. 
      
  2. JavaScript,

    ``'#'``
    
  3. ``javascript:`` URL
    
  4. 2.6 is feature frozen. Can you change this to 2.7?

  5. The right side can be reformatted to fit more on the first line.

  6. docs/manual/extending/extensions/hooks/action-hook.rst (Diff revision 15)
     
     
     
     
     

    The from should be lined up with code.

  7. reviewboard/extensions/hooks.py (Diff revision 15)
     
     
     
     
     
     
     
     

    Please format this as:

    from djblets.extensions.hooks import (AppliesToURLMixin, DataGridColumnsHook,
                                          ExtensionHook, ExtensionHookPoint,
                                          SignalHook, TemplateHook, URLHook)
    
  8. reviewboard/extensions/hooks.py (Diff revision 15)
     
     
     

    Format this as:

            super(BaseReviewRequestActionHook, self).__init__(
                extension, apply_to=apply_to, *args, **kwargs)
    
  9. reviewboard/extensions/hooks.py (Diff revision 15)
     
     

    Needs Args and Returns, e.g.

    """Single line summary.
    
    Multi-line description.
    
    Args:
        actions (list):
            A brief description of what actions is.
    
    Returns:
        list:
        A brief description of what the return value is.
    """
    
  10. reviewboard/extensions/hooks.py (Diff revision 15)
     
     
     

    Blank line between these.

  11. reviewboard/extensions/hooks.py (Diff revision 15)
     
     

    So registered_actions is a temporary that is only used in this function, but we're creating a iterator out of it (via reversed()) and then turning it into a new list. This is rather inefficient.

    Instead, can we do:

    registered_actions.reverse()
    return registered_actions
    

    This way, registered_actions will be reversed in-place and so we won't waste time and memory creating a new list and freeing the old one.

  12. reviewboard/extensions/hooks.py (Diff revision 15)
     
     

    Needs Args and Returns.

  13. reviewboard/extensions/hooks.py (Diff revision 15)
     
     
     
     
     
     
     
     
     
     
     

    This should be a class member, or a global member. I'm not sure.

    But it definitely shouldn't be in this method.

  14. reviewboard/extensions/hooks.py (Diff revision 15)
     
     

    Needs a better docstring to explain why this is necessary.

  15. reviewboard/extensions/hooks.py (Diff revision 15)
     
     

    Should be done in __init__.

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

    Please format as:

            super(ReviewRequestActionHook, self).__init__(
                extension, apply_to, actions, *args, **kwargs)
    
  17. reviewboard/extensions/hooks.py (Diff revision 15)
     
     
     
     
     
     
     
     
     
     

    Likewise, I don't think this should be in the def.

  18. reviewboard/extensions/hooks.py (Diff revision 15)
     
     
     
     
     
     

    Format as:

            return TempMenuAction([
                super(ReviewRequestDropdownActionHook, self)._to_action_instance(
                    child_action_dict)
                for child_action_dict in action_dict['items']
            ])
    
  19. reviewboard/extensions/tests.py (Diff revision 15)
     
     
     

    Alphabetize these imports.

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

    Put this on the previous line.

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

    Put this on the previous line.

  22. reviewboard/extensions/tests.py (Diff revision 15)
     
     

    Put this on the previous line.

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

    Put this on the previous line.

  24. reviewboard/extensions/tests.py (Diff revision 15)
     
     

    Put this on the previous line.

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

    Put this on the previous line.

  26. reviewboard/extensions/tests.py (Diff revision 15)
     
     

    Needs a docstring.

  27. reviewboard/extensions/tests.py (Diff revision 15)
     
     
     
     
     
     
     
     
     
     
     
     
     

    Undo this.

  28. reviewboard/extensions/tests.py (Diff revision 15)
     
     
     
     
     
     
     
     
     
     
     
     
     

    Undo this.

  29. reviewboard/extensions/tests.py (Diff revision 15)
     
     
     
     
     
     
     
     
     
     
     
     
     

    Undo this.

  30. reviewboard/reviews/actions.py (Diff revision 15)
     
     
     
     
     
     
     

    The text should line up with the n in note.

  31. reviewboard/reviews/actions.py (Diff revision 15)
     
     
     

    Can you format this as:

    parent (BaseReviewRequestMenuAction):
        The parent action instance of this action instance (optional).
    
  32. reviewboard/reviews/actions.py (Diff revision 15)
     
     
     
     
     
     

    Text should align with n in note.

  33. reviewboard/reviews/actions.py (Diff revision 15)
     
     

    Put on the previous line.

  34. reviewboard/reviews/actions.py (Diff revision 15)
     
     

    Should be a complete sentence.

  35. reviewboard/reviews/actions.py (Diff revision 15)
     
     

    Docstring should mention that this is a generator.

  36. reviewboard/reviews/actions.py (Diff revision 15)
     
     
     
     

    """
    Yields:
        BaseReviewREquestActions:
        All top-level registered review request action instances.
    """
    
  37. reviewboard/reviews/actions.py (Diff revision 15)
     
     
     
     

    Format as:

    """
    KeyError:
        ...
    """
    
  38. reviewboard/reviews/actions.py (Diff revision 15)
     
     

    Should be complete sentence.

  39. reviewboard/reviews/default_actions.py (Diff revision 15)
     
     

    "Determines" over "Tracks"

  40. reviewboard/reviews/default_actions.py (Diff revision 15)
     
     

    Put this on the previous line please.

  41. reviewboard/reviews/default_actions.py (Diff revision 15)
     
     

    "otherwise" instead of "else".

  42. reviewboard/reviews/default_actions.py (Diff revision 15)
     
     

    Needs Returns.

  43. 
      
AD
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/extensions/tests.py
        reviewboard/extensions/hooks.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/default_actions.py
        reviewboard/staticbundles.py
        reviewboard/extensions/templatetags/rb_extensions.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        docs/manual/extending/coderef/index.rst
        reviewboard/templates/reviews/ui/base.html
        reviewboard/static/rb/js/extensions/models/reviewRequestActionHookModel.js
        docs/manual/extending/extensions/hooks/index.rst
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/extensions/action_dropdown.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        docs/manual/extending/extensions/hooks/action-hook.rst
        reviewboard/templates/reviews/action.html
        docs/manual/extending/extensions/hooks/action-hooks.rst
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/templates/reviews/menu_action.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/extensions/tests.py
        reviewboard/extensions/hooks.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/default_actions.py
        reviewboard/staticbundles.py
        reviewboard/extensions/templatetags/rb_extensions.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        docs/manual/extending/coderef/index.rst
        reviewboard/templates/reviews/ui/base.html
        reviewboard/static/rb/js/extensions/models/reviewRequestActionHookModel.js
        docs/manual/extending/extensions/hooks/index.rst
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/extensions/action_dropdown.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        docs/manual/extending/extensions/hooks/action-hook.rst
        reviewboard/templates/reviews/action.html
        docs/manual/extending/extensions/hooks/action-hooks.rst
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/templates/reviews/menu_action.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
AD
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/extensions/tests.py
        reviewboard/extensions/hooks.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/default_actions.py
        reviewboard/staticbundles.py
        reviewboard/extensions/templatetags/rb_extensions.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        docs/manual/extending/coderef/index.rst
        reviewboard/templates/reviews/ui/base.html
        reviewboard/static/rb/js/extensions/models/reviewRequestActionHookModel.js
        docs/manual/extending/extensions/hooks/index.rst
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/extensions/action_dropdown.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        docs/manual/extending/extensions/hooks/action-hook.rst
        reviewboard/templates/reviews/action.html
        docs/manual/extending/extensions/hooks/action-hooks.rst
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/templates/reviews/menu_action.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/extensions/tests.py
        reviewboard/extensions/hooks.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/default_actions.py
        reviewboard/staticbundles.py
        reviewboard/extensions/templatetags/rb_extensions.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        docs/manual/extending/coderef/index.rst
        reviewboard/templates/reviews/ui/base.html
        reviewboard/static/rb/js/extensions/models/reviewRequestActionHookModel.js
        docs/manual/extending/extensions/hooks/index.rst
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/extensions/action_dropdown.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        docs/manual/extending/extensions/hooks/action-hook.rst
        reviewboard/templates/reviews/action.html
        docs/manual/extending/extensions/hooks/action-hooks.rst
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/templates/reviews/menu_action.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
AD
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/extensions/tests.py
        reviewboard/extensions/hooks.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/default_actions.py
        reviewboard/staticbundles.py
        reviewboard/extensions/templatetags/rb_extensions.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        docs/manual/extending/coderef/index.rst
        reviewboard/templates/reviews/ui/base.html
        reviewboard/static/rb/js/extensions/models/reviewRequestActionHookModel.js
        docs/manual/extending/extensions/hooks/index.rst
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/extensions/action_dropdown.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        docs/manual/extending/extensions/hooks/action-hook.rst
        reviewboard/templates/reviews/action.html
        docs/manual/extending/extensions/hooks/action-hooks.rst
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/templates/reviews/menu_action.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/extensions/tests.py
        reviewboard/extensions/hooks.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/default_actions.py
        reviewboard/staticbundles.py
        reviewboard/extensions/templatetags/rb_extensions.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        docs/manual/extending/coderef/index.rst
        reviewboard/templates/reviews/ui/base.html
        reviewboard/static/rb/js/extensions/models/reviewRequestActionHookModel.js
        docs/manual/extending/extensions/hooks/index.rst
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/extensions/action_dropdown.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        docs/manual/extending/extensions/hooks/action-hook.rst
        reviewboard/templates/reviews/action.html
        docs/manual/extending/extensions/hooks/action-hooks.rst
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/templates/reviews/menu_action.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
chipx86
  1. This is fantastic. Now, my review is large, because I'm in nit-picky mode, and want to get everything super-polished.

    There are a couple of larger discussion points, which I cover below. Basically, the actions are setting state at runtime on themselves, but the actions are instances shared across every user rendering the page. This will lead to bad state at times, and so must be redesigned a little.

    I also want to see more unit tests covering all the base action behavior, for registering/unregistering/iterating through actions (and handling all the success/error states), and unit tests for each default action covering the possible states. This will ensure things don't accidentally break down the road.

  2. docs/manual/extending/extensions/hooks/action-hook.rst (Diff revision 18)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    I'd rather not use a table for this (and require the shorthand for the actions). A bullet point list is sufficient.

    Same below.

  3. docs/manual/extending/extensions/hooks/action-hook.rst (Diff revision 18)
     
     
     
     
     
     

    The description shouldn't be on the same line as the version number. Look around the docs codebase and you'll find some examples of how this should look.

  4. Two blank lines between sections.

    Same below.

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

    What's this for?

    Ideally, all named lists of URLs should be defined in exactly one place, reviewboard.urls, so depending on what this is, maybe it should live there.

    1. It's passed in to the apply_to attribute of ReviewRequestActionHook, so that it only renders on the review request page (and not the file attachment or diff viewer pages). I'll go ahead and move it there.

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

    This should spend some time describing what actions are and roughly how they work, like the old docs did.

  7. reviewboard/extensions/hooks.py (Diff revision 18)
     
     
     
     
     
     
     
     
     
     
     
     

    I'd prefer not to align like this. Instead:

    * ``id``:
      The ID of the action (optional).
    
    
    * ``label``:
      ...
    

    (Make sure that renders correctly, though.. there are other docs that do something similar to this, so they can be used as a base.)

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

    Blank line between these.

  9. reviewboard/extensions/hooks.py (Diff revision 18)
     
     

    This needs a docstring.

  10. reviewboard/extensions/hooks.py (Diff revision 18)
     
     

    This definitely need more docs. I don't have a good sense of what this does.

  11. reviewboard/extensions/hooks.py (Diff revision 18)
     
     

    This is assuming some things about the data passed in, and if the keys are missing, the caller's going to get a less-than-nice exception. Ideally, this function should verify that action_dict has all the keys needed, and raise a KeyError with a friendly message if one is missing.

  12. reviewboard/extensions/hooks.py (Diff revision 18)
     
     
     

    This looks as if the self.label... is a third parameter. I'd instead do:

    self.action_id = action_dict.get(
        'id',
        ...)
    
  13. reviewboard/extensions/hooks.py (Diff revision 18)
     
     

    This should also be fleshed out more.

  14. reviewboard/extensions/hooks.py (Diff revision 18)
     
     
     
     

    Same stuff as above.

  15. reviewboard/extensions/hooks.py (Diff revision 18)
     
     
     

    These are best referenced using :guilabel:.

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

    Maybe list these, so a caller doesn't have to look through the class hierarchy and check parent classes.

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

    Needs a docstring.

  18. reviewboard/extensions/hooks.py (Diff revision 18)
     
     
     

    Instead of setting these, just pass them inline where needed.

  19. reviewboard/extensions/hooks.py (Diff revision 18)
     
     

    This is kind of named awkwardly. From the name, I have no real sense of what it's doing, and the description doesn't really cover it any further.

    Looking through the code, I believe it's taking the list passed in and creating a new list of normalized values. So, maybe _normalize_actions, with a description saying what it's taking in and what it's turning them into.

  20. reviewboard/extensions/hooks.py (Diff revision 18)
     
     

    :py:class: for dict.

  21. reviewboard/extensions/hooks.py (Diff revision 18)
     
     

    BaseReviewRequestAction

  22. reviewboard/extensions/hooks.py (Diff revision 18)
     
     

    I notice we're reversing here, and then reversing again later. What's the reason for that?

    1. Since newly registered top-level actions are appended to the left of the other previously registered top-level actions, we must iterate through the actions in reverse. However, we don't want to mutate the original actions and we want to return the same (possibly normalized) actions in their original order. Hence, we reverse twice in this method.


      I'll add the above explanation to the docstring. (Of course...it's possible that appending to the left of the _top_level_ids deque might be a bad design in the first place, and that we should perhaps be storing the top level action IDs in reverse instead. Do you have any thoughts?)

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

    What other values can this be? It might want to sanity-check that everything provided is an expected type of value, and raise an exception otherwise.

  24. reviewboard/extensions/hooks.py (Diff revision 18)
     
     
     

    Blank line between these.

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

    A function beginning with to_... generally means that the owning class would be converted to a thing. Let's go with _convert_action or something.

  26. reviewboard/extensions/hooks.py (Diff revision 18)
     
     

    Let's flesh this out a bit, mention that this means it'll only show up on the review request page containing reviews. I don't know that we want to mention "tabs" so much, as we're really dealing with pages, and who's to say whether tabs are the UI of choice for the product in all forms down the road.

    Same below.

  27. reviewboard/extensions/hooks.py (Diff revision 18)
     
     

    Needs a docstring.

  28. reviewboard/extensions/hooks.py (Diff revision 18)
     
     
     

    Same above. We should list the possible types.

  29. reviewboard/extensions/hooks.py (Diff revision 18)
     
     
     
     

    Same as above with the formatting.

  30. reviewboard/extensions/hooks.py (Diff revision 18)
     
     

    This should go in an Example section, like:

    """
    
    ...
    
    Example:
        .. code-block:: python
    
            actions = [{
                ...
            }]
    
  31. reviewboard/extensions/hooks.py (Diff revision 18)
     
     

    Same naming thoughts as above.

    Also, needs docstrings.

  32. reviewboard/extensions/hooks.py (Diff revision 18)
     
     
     
     

    Same comments as above.

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

    This is one of the older unit test suites in the codebase. Let's go ahead and move all action unit tests into a new class. This makes it easier for us to run the test suite only for action hook functionality.

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

    I know the old unit test names didn't do this, but ideally, all new unit test names should reference the class being used for the test.

    "Testing ReviewRequestActionHook with ..."

    That sort of thing.

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

    These are nice cleanups, but really should be broken out into its own change, to help keep this change slimmer.

    Generally speaking, cleanup changes should be standalone. When running a git blame in the future, and seeing a change to this line, I'll wonder how it relates to the action hook work. If instead the change says it's standardizing names for something, then great!

    That said, I don't know that this is cleanup that needs to happen (I'd rather see it done piece-meal as this mega-class is broken up into smaller test suite classes), so my preference would be to undo it.

  36. reviewboard/reviews/actions.py (Diff revision 18)
     
     

    This should go in an "Example:" section, like:

    """
    ...
    
    Example:
    
        .. code-block:: python:
    
            class ....
    """
    

    We've been placing these after things like attributes, as well.

  37. reviewboard/reviews/actions.py (Diff revision 18)
     
     
     
     
     
     
     
     
     
     
     
     
     

    These should be documented inline where defined on the class. "Attributes:" is more useful when documenting things only set in __init__ that are meant as public API.

  38. reviewboard/reviews/actions.py (Diff revision 18)
     
     
     

    Private attributes shouldn't be documented.

    Same below.

  39. reviewboard/reviews/actions.py (Diff revision 18)
     
     
     
     
     

    If a subclass is meant to override this, it should not be a private function.

  40. reviewboard/reviews/actions.py (Diff revision 18)
     
     

    Should be in a "Note:" section instead.

  41. reviewboard/reviews/actions.py (Diff revision 18)
     
     
     

    Should be indented one level.

  42. reviewboard/reviews/actions.py (Diff revision 18)
     
     

    Same here.

  43. reviewboard/reviews/actions.py (Diff revision 18)
     
     
     
     
     
     

    You can combine these to be optimistic:

    try:
        del _all_actions[...]
    except KeyError:
        raise KeyError(...)
    
  44. reviewboard/reviews/actions.py (Diff revision 18)
     
     
     
     
     
     
     
     
     
     
     
     
     

    Not worth repeating these.

    Same below.

  45. reviewboard/reviews/actions.py (Diff revision 18)
     
     

    Needs a docstring.

  46. reviewboard/reviews/actions.py (Diff revision 18)
     
     
     

    Blank line between these, generally, but this might be better as:

    try:
        parent = _all_actions[parent_id]
    except KeyError:
        raise KeyError(...)
    
  47. reviewboard/reviews/actions.py (Diff revision 18)
     
     
     
     
     

    Same as above.

  48. reviewboard/reviews/actions.py (Diff revision 18)
     
     

    Let's flesh this out just a bit, describing the resulting state.

    May also need a warning that this will impact any extensions that have registered actions. (In fact, the extension hooks should probably become more bullet-proof, catching unregistration errors and logging them, instead of blowing up.)

  49. reviewboard/reviews/default_actions.py (Diff revision 18)
     
     
     

    Blank line before blocks.

  50. reviewboard/reviews/default_actions.py (Diff revision 18)
     
     
     

    Blank line between these.

    Though, you might want to just combine the statements.

  51. reviewboard/reviews/default_actions.py (Diff revision 18)
     
     
     

    Let's have the third condition on its own line.

  52. reviewboard/reviews/default_actions.py (Diff revision 18)
     
     

    So I have a concern here, and I want to follow up on how this works.

    We're registering instances, right? So if this is doing self.label = ... anywhere, it can pollute other renders (as the same codebase is going to be serving up multiple people). That's going to be a real problem.

    Basically, if we're registering instances, rather than having an instance tied to that page load, then we cannot ever set anything on the class at runtime. We instead need functions that return the values needed.

    In that case, we'd want get_label(), get_url(), etc. that take a context and return a value, much like should_render().

  53. reviewboard/reviews/default_actions.py (Diff revision 18)
     
     
     

    Blank line between these.

  54. reviewboard/reviews/default_actions.py (Diff revision 18)
     
     
     

    Here too.

    Though, you may want to just combine the statements.

  55. reviewboard/reviews/default_actions.py (Diff revision 18)
     
     

    Parens around the comparison.

  56. reviewboard/reviews/default_actions.py (Diff revision 18)
     
     

    Description on the next line.

    1. I talked to Barret about this on Slack here. Since this description fits on a single line, I think this is fine as it stands.

  57. This might need to provide an example.

  58. If we're changing the names, let's get rid of -link and replace it with -action.

  59. No space after function.

  60. 
      
AD
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/extensions/tests.py
        reviewboard/urls.py
        reviewboard/extensions/hooks.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/default_actions.py
        reviewboard/staticbundles.py
        reviewboard/extensions/templatetags/rb_extensions.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        docs/manual/extending/coderef/index.rst
        reviewboard/templates/reviews/ui/base.html
        reviewboard/static/rb/js/extensions/models/reviewRequestActionHookModel.js
        docs/manual/extending/extensions/hooks/index.rst
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/extensions/action_dropdown.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        docs/manual/extending/extensions/hooks/action-hook.rst
        reviewboard/templates/reviews/action.html
        docs/manual/extending/extensions/hooks/action-hooks.rst
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/templates/reviews/menu_action.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/extensions/tests.py
        reviewboard/urls.py
        reviewboard/extensions/hooks.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/default_actions.py
        reviewboard/staticbundles.py
        reviewboard/extensions/templatetags/rb_extensions.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        docs/manual/extending/coderef/index.rst
        reviewboard/templates/reviews/ui/base.html
        reviewboard/static/rb/js/extensions/models/reviewRequestActionHookModel.js
        docs/manual/extending/extensions/hooks/index.rst
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/extensions/action_dropdown.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        docs/manual/extending/extensions/hooks/action-hook.rst
        reviewboard/templates/reviews/action.html
        docs/manual/extending/extensions/hooks/action-hooks.rst
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/templates/reviews/menu_action.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. reviewboard/extensions/tests.py (Diff revision 19)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
AD
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/extensions/tests.py
        reviewboard/urls.py
        reviewboard/extensions/hooks.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/default_actions.py
        reviewboard/staticbundles.py
        reviewboard/extensions/templatetags/rb_extensions.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        docs/manual/extending/coderef/index.rst
        reviewboard/templates/reviews/ui/base.html
        reviewboard/static/rb/js/extensions/models/reviewRequestActionHookModel.js
        docs/manual/extending/extensions/hooks/index.rst
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/extensions/action_dropdown.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        docs/manual/extending/extensions/hooks/action-hook.rst
        reviewboard/templates/reviews/action.html
        docs/manual/extending/extensions/hooks/action-hooks.rst
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/templates/reviews/menu_action.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/actions.py
        reviewboard/extensions/tests.py
        reviewboard/urls.py
        reviewboard/extensions/hooks.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/default_actions.py
        reviewboard/staticbundles.py
        reviewboard/extensions/templatetags/rb_extensions.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        docs/manual/extending/coderef/index.rst
        reviewboard/templates/reviews/ui/base.html
        reviewboard/static/rb/js/extensions/models/reviewRequestActionHookModel.js
        docs/manual/extending/extensions/hooks/index.rst
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/extensions/action_dropdown.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        docs/manual/extending/extensions/hooks/action-hook.rst
        reviewboard/templates/reviews/action.html
        docs/manual/extending/extensions/hooks/action-hooks.rst
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/templates/reviews/menu_action.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. reviewboard/reviews/default_actions.py (Diff revision 20)
     
     
    Col: 17
     W503 line break before binary operator
    
  3. 
      
AD