Implement review request action registration.
Review Request #7661 — Created Sept. 26, 2015 and submitted
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. |
|
|
Col: 80 E501 line too long (80 > 79 characters) |
![]() |
|
Ideally, the should_render function should be a member funciton. This way, the people using this code can override the function … |
|
|
These should probably all be class attributes. |
|
|
Re: my earlier comment, this would be class EditReviewAction(ReviewRequestAction): """...""" def should_render(self, request): return request.user.is_authenticated() |
|
|
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 … |
|
|
Style nit: blank line between these. |
|
|
BLank line between these. |
|
|
This file should start with from __future__ import unicode_literals |
|
|
Docstrings should be in the imperative mood ("Return" vs "Returns") |
|
|
Imperative mood ("Populates" -> "Populate") |
|
|
Returns -> Return |
|
|
Registers -> Register |
|
|
This file should start with the unicode_literals import. |
|
|
This will need to be able to change based on what revision of the diff a user is looking at. |
|
|
Place the comment above this and prefix it with a colon, e.g.: #: A mapping of action IDs to action … |
|
|
Likewise here. However, this doesn't need a #: comment. |
|
|
Can you give this a list of arguments? e.g. """.... Args: arg (arg_type): Brief description of arg. """ This comment … |
|
|
:py:exc:KeyError |
|
|
Blank line between these. |
|
|
We prefer %-expressions for formatting over ''.format. |
|
|
You should have a constant on the class, e.g. class DownloadDiffAction(BaseReviewRequestAction): # ... #: Brief comment descrbing what this does. … |
|
|
Blank line between statement and block. |
|
|
This needs a better variable name. Perhaps, content? |
|
|
:py:class:`BaseReviewRequestAction` |
|
|
This should use reST clode blocks. E.g., For example: .. code-block:: python class UsedOnceAction(BaseReviewRequestAction): action_id = 'once' label = 'This … |
|
|
Blank line between these. |
|
|
This should go above args, e.g.: """Return whether or not this action should be rendered. The default implementation is to … |
|
|
Blank line between these. |
|
|
This should go above Args |
|
|
This should just be KeyError. The :py:exc:Exception stuff is for inline in strings, not in things that the docs expect … |
|
|
Needs a docstring. Likewise for all members of this class. |
|
|
Needs a docstring. Also, blank line between this and action_id. Likewise for encapsulated classes. |
|
|
This should be above args. |
|
|
No blank line between the docstring and the first line of code for functions. |
|
|
No blank line between the docstring and the first line of code for functions. |
|
|
Because you're manipulating this in a method, it probably shouldn't be data on the class. Can you create an __init__ … |
|
|
No blank line between the docstring and the first line of code for functions. |
|
|
No blank line between the docstring and the first line of code for functions. |
|
|
No blank line between the docstring and the first line of code for functions. |
|
|
No blank line between the docstring and the first line of code for functions. |
|
|
This comment confuses me. |
|
|
Can we call this review_request? We don't abbreviate to "rev" this way. |
|
|
You should be able to just use .display_id here. |
|
|
No blank line between docstring and code in functions. |
|
|
These two lines could be combined (you only use actions once) |
|
|
Hmm. I'm not sure how I feel about passing in a list and having the render() method append to it. … |
|
|
Let's put the whole if/endif thing on the same line. |
|
|
Add another blank line after this. |
|
|
I'm not sure why all of these are class methods. Can you clarify for me? |
|
|
Can we just pull these out into top-level classes? There's no real reason to hide them inside their parent menus, … |
|
|
"children_actions" is kind of redundant. How about either "children" or "child_actions"? |
|
|
Same with these (pull them out into top-level classes). |
|
|
This is probably not a terribly reliable way to detect this. It might be a lot better to pass the … |
|
|
We should maybe also pass the request into this, and introspect the url/view name, instead of trying to reverse engineer … |
|
|
Can we wrap this somehow? Maybe split the line after the href="..."? |
|
|
Col: 1 E302 expected 2 blank lines, found 1 |
![]() |
|
Yes, it should be. __all__ defines which symbols get exported from the file (if there's no __all__, everything is exported). |
|
|
_() doesn't work this way. The l10n scanner looks at the source code and extracts all string literals which are … |
|
|
Same here. |
|
|
I would wrap this differently: super(DiffViewerActionHook, self).__init__( extension, apply_to, actions, *args, **kwargs) |
|
|
Add a blank line after this. |
|
|
Because we're intending to return a bool (and not a Repository), we should make this a truth test: return review_request.repository … |
|
|
Instead of using 'P' as a literal, you can compare this against ReviewRequest.PENDING_REVIEW. |
|
|
'_' imported but unused |
![]() |
|
Mind fixing the spelling here while you're editing this file? |
|
|
It doesn't really matter, but I think it would be nicer to have this set after we register all the … |
|
|
Instead of just casting to bool, I think we should explicitly test review_request.repository is not None. |
|
|
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) |
|
|
Same here. |
|
|
URL. |
|
|
JavaScript, ``'#'`` |
|
|
``javascript:`` URL |
|
|
2.6 is feature frozen. Can you change this to 2.7? |
|
|
The right side can be reformatted to fit more on the first line. |
|
|
Again, 2.7. |
|
|
2.7 |
|
|
The from should be lined up with code. |
|
|
Please format this as: from djblets.extensions.hooks import (AppliesToURLMixin, DataGridColumnsHook, ExtensionHook, ExtensionHookPoint, SignalHook, TemplateHook, URLHook) |
|
|
Format this as: super(BaseReviewRequestActionHook, self).__init__( extension, apply_to=apply_to, *args, **kwargs) |
|
|
Needs Args and Returns, e.g. """Single line summary. Multi-line description. Args: actions (list): A brief description of what actions is. … |
|
|
Blank line between these. |
|
|
So registered_actions is a temporary that is only used in this function, but we're creating a iterator out of it … |
|
|
Needs Args and Returns. |
|
|
This should be a class member, or a global member. I'm not sure. But it definitely shouldn't be in this … |
|
|
Needs a better docstring to explain why this is necessary. |
|
|
Should be done in __init__. |
|
|
Please format as: super(ReviewRequestActionHook, self).__init__( extension, apply_to, actions, *args, **kwargs) |
|
|
Likewise, I don't think this should be in the def. |
|
|
Format as: return TempMenuAction([ super(ReviewRequestDropdownActionHook, self)._to_action_instance( child_action_dict) for child_action_dict in action_dict['items'] ]) |
|
|
Alphabetize these imports. |
|
|
Put this on the previous line. |
|
|
Put this on the previous line. |
|
|
Put this on the previous line. |
|
|
Put this on the previous line. |
|
|
Put this on the previous line. |
|
|
Put this on the previous line. |
|
|
Needs a docstring. |
|
|
Undo this. |
|
|
Undo this. |
|
|
Undo this. |
|
|
The text should line up with the n in note. |
|
|
Can you format this as: parent (BaseReviewRequestMenuAction): The parent action instance of this action instance (optional). |
|
|
Text should align with n in note. |
|
|
Put on the previous line. |
|
|
Should be a complete sentence. |
|
|
Docstring should mention that this is a generator. |
|
|
""" Yields: BaseReviewREquestActions: All top-level registered review request action instances. """ |
|
|
Format as: """ KeyError: ... """ |
|
|
Should be complete sentence. |
|
|
"Determines" over "Tracks" |
|
|
Put this on the previous line please. |
|
|
"otherwise" instead of "else". |
|
|
Needs Returns. |
|
|
I'd rather not use a table for this (and require the shorthand for the actions). A bullet point list is … |
|
|
The description shouldn't be on the same line as the version number. Look around the docs codebase and you'll find … |
|
|
Two blank lines between sections. Same below. |
|
|
What's this for? Ideally, all named lists of URLs should be defined in exactly one place, reviewboard.urls, so depending on … |
|
|
This should spend some time describing what actions are and roughly how they work, like the old docs did. |
|
|
I'd prefer not to align like this. Instead: * ``id``: The ID of the action (optional). * ``label``: ... (Make … |
|
|
Blank line between these. |
|
|
This needs a docstring. |
|
|
This definitely need more docs. I don't have a good sense of what this does. |
|
|
This is assuming some things about the data passed in, and if the keys are missing, the caller's going to … |
|
|
This looks as if the self.label... is a third parameter. I'd instead do: self.action_id = action_dict.get( 'id', ...) |
|
|
This should also be fleshed out more. |
|
|
Same stuff as above. |
|
|
These are best referenced using :guilabel:. |
|
|
Maybe list these, so a caller doesn't have to look through the class hierarchy and check parent classes. |
|
|
Needs a docstring. |
|
|
Instead of setting these, just pass them inline where needed. |
|
|
This is kind of named awkwardly. From the name, I have no real sense of what it's doing, and the … |
|
|
:py:class: for dict. |
|
|
BaseReviewRequestAction |
|
|
I notice we're reversing here, and then reversing again later. What's the reason for that? |
|
|
What other values can this be? It might want to sanity-check that everything provided is an expected type of value, … |
|
|
Blank line between these. |
|
|
A function beginning with to_... generally means that the owning class would be converted to a thing. Let's go with … |
|
|
Let's flesh this out a bit, mention that this means it'll only show up on the review request page containing … |
|
|
Needs a docstring. |
|
|
Same above. We should list the possible types. |
|
|
Same as above with the formatting. |
|
|
This should go in an Example section, like: """ ... Example: .. code-block:: python actions = [{ ... }] |
|
|
Same naming thoughts as above. Also, needs docstrings. |
|
|
Same comments as above. |
|
|
This is one of the older unit test suites in the codebase. Let's go ahead and move all action unit … |
|
|
I know the old unit test names didn't do this, but ideally, all new unit test names should reference the … |
|
|
These are nice cleanups, but really should be broken out into its own change, to help keep this change slimmer. … |
|
|
This should go in an "Example:" section, like: """ ... Example: .. code-block:: python: class .... """ We've been placing … |
|
|
These should be documented inline where defined on the class. "Attributes:" is more useful when documenting things only set in … |
|
|
Private attributes shouldn't be documented. Same below. |
|
|
If a subclass is meant to override this, it should not be a private function. |
|
|
Should be in a "Note:" section instead. |
|
|
Should be indented one level. |
|
|
Same here. |
|
|
You can combine these to be optimistic: try: del _all_actions[...] except KeyError: raise KeyError(...) |
|
|
Not worth repeating these. Same below. |
|
|
Needs a docstring. |
|
|
Blank line between these, generally, but this might be better as: try: parent = _all_actions[parent_id] except KeyError: raise KeyError(...) |
|
|
Same as above. |
|
|
Let's flesh this out just a bit, describing the resulting state. May also need a warning that this will impact … |
|
|
Blank line before blocks. |
|
|
Blank line between these. Though, you might want to just combine the statements. |
|
|
Let's have the third condition on its own line. |
|
|
So I have a concern here, and I want to follow up on how this works. We're registering instances, right? … |
|
|
Blank line between these. |
|
|
Here too. Though, you may want to just combine the statements. |
|
|
Parens around the comparison. |
|
|
Description on the next line. |
|
|
This might need to provide an example. |
|
|
This should be documented. |
|
|
If we're changing the names, let's get rid of -link and replace it with -action. |
|
|
No space after function. |
|
|
Col: 80 E501 line too long (80 > 79 characters) |
![]() |
|
Col: 17 W503 line break before binary operator |
![]() |
|
'register_actions' imported but unused |
![]() |
|
'unregister_actions' imported but unused |
![]() |
|
A form we're moving to, which reads and maintains better, is: ``id`` (optional): The ID of... ``label``: ... |
|
|
Only two backticks are needed. |
|
|
Any place we're referencing some key, we should use double backticks instead of making it bold. |
|
|
Not sure that we should document registering without a hook in an extension, since that means more that the extension … |
|
|
We should document that this works on all review request pages. |
|
|
This is probably not something we want to explicitly document here. Instead, we should somehow save and restore the actions … |
|
|
Like with the docs, let's do: ``id`` (optional): ... ``label``: ... |
|
|
Can you add ", optional" to the end of this, inside the parens? Same with any other arguments you have … |
|
|
Technically, this is a tuple. Same below. |
|
|
The type should be "callable". |
|
|
It's not generally good for classes to override functions like this. Let's define a proper method instead. Same below. |
|
|
I'm actually not sure, what is #. ? |
|
|
Should have ", optional". |
|
|
"Raises" should go after "Returns". Same below. |
|
|
We should use :py:class: here. Same below. |
|
|
No indentation for the description here. Same below. |
|
|
Maybe add a comment saying why we do this in reverse order. |
|
|
Can return reversed(registered_actions) instead. |
|
|
Should have :py:class on the class names in docstrings. Same elsewhere. |
|
|
Should have ", optional" |
|
|
Same as above with the formatting. |
|
|
No period on unit test docstrings. Same below. |
|
|
Let's specify the base class name. |
|
|
Let's specify the class name. |
|
|
I think "height" might actually be a bit too confusing. "Depth" worked better. "Height" usually means the dimensional height of … |
|
|
Maybe "as HTML." ? |
|
|
Should have ", optional". Then there's no need for "(optional)" at the end. |
|
|
Should wrap this in a try:, in case there's a failure, and log in except:. Then we can pop in … |
|
|
No need for the inner parens. |
|
|
% should go on the next line. Not sure RuntimeError is right here. Is this something that should never happen, … |
|
|
No need for the inner parens. |
|
|
", optional" |
|
|
"as HTML." Same with any others. |
|
|
", optional" |
|
|
This comment isn't about this line per se, but we have this nifty new Registry thing that Barret put together … |
|
|
", optional" |
|
|
No need for inner parens. |
|
|
Blank line between these. |
|
|
As mentioned earlier in the review, this is kind of dangerous for extensions, because they can stomp all over each … |
|
|
We can now use @register.simple_tag(takes_context=True). @basictag is newly-deprecated. Same below. |
|
|
Blank line between these. Same with ones below. |
|
|
We should probably call this in tearDown(). |
|
|
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 |
|
|
Instead of a list (since this is "only" one thing), maybe call this main_review_request_url_name? |
|
-
-
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).
-
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?
-
-
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.
-
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 returnTrue
) to do complicated stuff with, e.g., therequest
. -
-
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()
-
reviewboard/reviews/builtin_actions.py (Diff revision 1) You should look at
reviewboard/accounts/pages.py
andreviewboard/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. -
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 1) Style nit: blank line between these.
-
Change Summary:
Address the issues raised by Review Bot, Mike, and Barret.
- Upgraded attributes from (instance, class) to (class, module) level.
- Made the url attribute default to be '#'.
- Renamed all "shipit-link" strings to "ship-it-link".
Description: |
|
||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+140 -7) |

-
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
-
-
reviewboard/reviews/actions.py (Diff revision 2) This file should start with
from __future__ import unicode_literals
-
reviewboard/reviews/actions.py (Diff revision 2) Docstrings should be in the imperative mood ("Return" vs "Returns")
-
-
-
-
reviewboard/reviews/builtin_actions.py (Diff revision 2) This file should start with the
unicode_literals
import. -
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.
Change Summary:
Address the issues raised by David.
- Imported
unicode_literals
for each new Python file. - Corrected the docstrings to use the imperative mood.
- Shortened the names of some of the default actions.
- Added the
hidden
class variable forDownloadDiffAction
. - Modified
view_diff.html
so that simple actions now work for both tabs.
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+175 -10) |

-
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
-
-
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'...') # ...
-
-
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 3) This needs a better variable name. Perhaps,
content
?
-
-
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.
-
reviewboard/reviews/actions.py (Diff revision 3) Likewise here. However, this doesn't need a
#:
comment. -
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. -
-
-
reviewboard/reviews/actions.py (Diff revision 3) We prefer
%-expressions
for formatting over''.format
.
Change Summary:
Address the issues raised by Barret.
- Switched to
%-expressions
over''.format()
. - Improved the docstrings (following a Google-ish standard).
- Added special
#:
type comments for auto-generated documentation. - Switched from registering actions classes to action instances.
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+230 -10) |

-
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
Change Summary:
Update the default menu actions.
- Changed from a block tag to a basic tag (
review_request_actions
). - Actions are now responsible for rendering via
context
. - Added a
menu_action.html
file that includesaction.html
.
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 5 (+355 -55) |

-
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
-
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.
-
-
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
-
-
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: ...
-
-
-
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. -
reviewboard/reviews/builtin_actions.py (Diff revision 5) Needs a docstring. Likewise for all members of this class.
-
reviewboard/reviews/builtin_actions.py (Diff revision 5) Needs a docstring. Also, blank line between this and
action_id
. Likewise for encapsulated classes. -
Change Summary:
Address the issues raised by Barret.
- Improved various comments.
- Added a
:py:class:
for classes in the multi-line part of a docstring. - Added a reST clode block for Python examples in the docstring.
- Added a newline after each docstring.
- Moved the Remark and Side Effect sections to the multi-line part.
- Renamed
builtin_actions
todefault_actions
. - Removed
:py:exc:
from the Raises section. - Added docstrings to several simple classes.
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+364 -55) |

-
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
Change Summary:
Improved the description for Part 3.
Description: |
|
---|
-
This looks like a great start!
-
reviewboard/reviews/actions.py (Diff revision 6) No blank line between the docstring and the first line of code for functions.
-
reviewboard/reviews/actions.py (Diff revision 6) No blank line between the docstring and the first line of code for functions.
-
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? -
reviewboard/reviews/actions.py (Diff revision 6) No blank line between the docstring and the first line of code for functions.
-
reviewboard/reviews/actions.py (Diff revision 6) No blank line between the docstring and the first line of code for functions.
-
reviewboard/reviews/actions.py (Diff revision 6) No blank line between the docstring and the first line of code for functions.
-
reviewboard/reviews/default_actions.py (Diff revision 6) No blank line between the docstring and the first line of code for functions.
-
-
reviewboard/reviews/default_actions.py (Diff revision 6) Can we call this
review_request
? We don't abbreviate to "rev" this way. -
reviewboard/reviews/default_actions.py (Diff revision 6) You should be able to just use
.display_id
here. -
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 6) No blank line between docstring and code in functions.
-
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 6) These two lines could be combined (you only use
actions
once) -
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 6) Hmm. I'm not sure how I feel about passing in a list and having the
render()
method append to it. How about we letrender()
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. -
reviewboard/templates/reviews/action.html (Diff revision 6) Let's put the whole if/endif thing on the same line.
Change Summary:
Address the issues raised by David.
- Removed blanklines after docstrings for functions.
- Changed
renderable_actions
to an instance attribute. - Improved various comments and variable names.
- Replaced various list comprehensions with generator expressions.
- Added Attributes sections for the base class docstrings.
- Made better use of default arguments and calling
super()
. - Implemented children action registration.
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 7 (+422 -55) |

-
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
Change Summary:
Allow extensions to provide their own actions (as dicts or instances).
- Removed 3 action hook basic tags:
1.review_request_action_hooks
2.review_request_dropdown_action_hooks
3.diffviewer_action_hooks
- Added a
BaseReviewRequestActionHook
for other hooks to inherit from. - Changed the
_top_level_ids
from alist
to adeque
.
- Registering an action involves appending to the left of adeque
, so
we sometimes have to iterate through actions in reverse.
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 8 (+598 -172) |

-
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
Change Summary:
Allow menu actions to have their own menu actions.
- Removed the
renderable_actions
attribute. - Made sure that menu actions check if they themselves should render
before recursively rendering its children actions. - Added a new
children_actions
basic tag.
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 9 (+605 -172) |

-
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
Change Summary:
Clean up comments and variable names and some redundant logic.
- Renamed
actions
tochildren_actions
. - Improved docstring explanation for registering actions.
- Removed
BaseReviewRequestMenuAction
's redundant logic inrender()
.
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 10 (+594 -172) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Added Files: |

-
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
-
-
-
reviewboard/extensions/hooks.py (Diff revision 10) I'm not sure why all of these are class methods. Can you clarify for me?
-
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.
-
reviewboard/reviews/default_actions.py (Diff revision 10) "children_actions" is kind of redundant. How about either "children" or "child_actions"?
-
reviewboard/reviews/default_actions.py (Diff revision 10) Same with these (pull them out into top-level classes).
-
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
. -
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.
-
reviewboard/templates/reviews/action.html (Diff revision 10) Can we wrap this somehow? Maybe split the line after the href="..."?
Change Summary:
Fix the commented out unit tests and implement unregistration.
- Both basic tags now have try/except blocks.
- Both base actions now have an
unregister
method (good for unit tests).
Use AppliesToURLMixin
and add one unit test.
BaseReviewRequestActionHook
now also subclassesAppliesToURLMixin
._action_should_render
was removed.
Address the issues raised by David.
- Replaced class methods with instance methods.
- Modified some public methods to be protected instead.
- Used
resolver_match
instead of relying on riskier context keys. - Renamed
children actions
->child_actions
.
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 11 (+747 -147) |

-
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
-
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 11) Col: 1 E302 expected 2 blank lines, found 1
Change Summary:
Fix whitespace and extend the base unit tests.

-
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
-
This is starting to look very solid!
-
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). -
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). -
-
reviewboard/extensions/hooks.py (Diff revision 12) I would wrap this differently:
super(DiffViewerActionHook, self).__init__( extension, apply_to, actions, *args, **kwargs)
-
-
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
-
reviewboard/reviews/default_actions.py (Diff revision 12) Instead of using
'P'
as a literal, you can compare this againstReviewRequest.PENDING_REVIEW
.
Change Summary:
Allow extensions to register actions with respect to a specified parent.
- Added a public
register_actions()
method. - Added detailed comments as to how I might refactor the code.
Address the issues raised by David.
- Stopped surrounding variables with
_()
. - Wrapped
_should_render
's return statements withbool()
. - Replaced literals with imported constants (sort of like an enum).
- Cleaned up some variable names and how default arguments are handled.
- Cleaned up various whitespace and wrapping conventions.
- Child actions can now be passed in via the constructor.
Allow extensions to unregister actions.
- Added a public
unregister_actions()
method. - Replaced
is_top_level
withparent
. - Added
_parent
so that extensions can unregister by ID.
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 13 (+849 -143)
|

-
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
-
-
-
reviewboard/extensions/tests.py (Diff revision 13) Mind fixing the spelling here while you're editing this file?
-
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.
-
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
. -
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 13) 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)
-
Change Summary:
Added a
reset_actions()
method (useful inshutdown()
for extensions).
- Replaced the
default_actions
variable with aget_default_actions()
factory method so that_populate_defaults()
can grab a fresh copy of
the original default actions after a reset.
Refactored when child actions get registered.
- Child actions now get registered as soon as they are passed in to the
parent menu action's constructor. So when the parent gets registered,
it no longer has to recursively register its children.
Added a unit test for modifying the default actions and cleaned up code.
- Reverted a change from the previous commit, since it led to infinite
recursion. - Fixed docstring omission.
- Replaced 'P' with constant.
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 14 (+899 -146)
|

-
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
Change Summary:
Update the documentation for action hooks.
- Used "note" blocks in various docstrings and fixed various whitespace
issues that were interfering with Sphinx. - Renamed
action-hooks
toaction-hook
. - Added
reviewboard.reviews.actions
and
reviewboard.reviews.default_actions
to thetoctree
. - Introduced abbreviations to help make the tables nicer.
- Added various "versionadded" and "seealso" and "autosummary" blocks.
- Added a new section for modifying the default actions.
- Extended the given example to illustrate the new functionality.
Add a ReviewRequestActionHook model for providing callbacks.
Summary: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 15 (+1141 -252)
|

-
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
-
-
-
-
-
docs/manual/extending/extensions/hooks/action-hook.rst (Diff revision 15) 2.6 is feature frozen. Can you change this to 2.7?
-
docs/manual/extending/extensions/hooks/action-hook.rst (Diff revision 15) The right side can be reformatted to fit more on the first line.
-
-
-
docs/manual/extending/extensions/hooks/action-hook.rst (Diff revision 15) The
from
should be lined up withcode
. -
reviewboard/extensions/hooks.py (Diff revision 15) Please format this as:
from djblets.extensions.hooks import (AppliesToURLMixin, DataGridColumnsHook, ExtensionHook, ExtensionHookPoint, SignalHook, TemplateHook, URLHook)
-
reviewboard/extensions/hooks.py (Diff revision 15) Format this as:
super(BaseReviewRequestActionHook, self).__init__( extension, apply_to=apply_to, *args, **kwargs)
-
reviewboard/extensions/hooks.py (Diff revision 15) Needs
Args
andReturns
, 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. """
-
-
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 (viareversed()
) 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. -
-
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.
-
reviewboard/extensions/hooks.py (Diff revision 15) Needs a better docstring to explain why this is necessary.
-
-
reviewboard/extensions/hooks.py (Diff revision 15) Please format as:
super(ReviewRequestActionHook, self).__init__( extension, apply_to, actions, *args, **kwargs)
-
reviewboard/extensions/hooks.py (Diff revision 15) Likewise, I don't think this should be in the
def
. -
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'] ])
-
-
-
-
-
-
-
-
-
-
-
-
-
reviewboard/reviews/actions.py (Diff revision 15) Can you format this as:
parent (BaseReviewRequestMenuAction): The parent action instance of this action instance (optional).
-
-
-
-
reviewboard/reviews/actions.py (Diff revision 15) Docstring should mention that this is a generator.
-
reviewboard/reviews/actions.py (Diff revision 15) """ Yields: BaseReviewREquestActions: All top-level registered review request action instances. """
-
-
-
-
-
-
Change Summary:
Address the issues raised by Barret and improve the ActionHook class.
- Improved various wording/whitespace/bracket/docstring issues.
- Changed Version 2.6 to Version 2.7.
- Used
~.parent_dir.
to help Sphinx find the correct hyperlinks. - Made
ActionHook
and other hooks more consistent with the docs/updates. - Extracted/improved the
DictAction
andDictMenuAction
helper classes. - Made wording more consistent for
ActionHook
-style and
ReviewRequestDropdownActionHook
-style dictionaries.
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 16 (+1258 -277)
|

-
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
Change Summary:
Add a couple of missing "Returns" docstring sections.
Summary: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 17 (+1264 -277)
|

-
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
Change Summary:
Alphabetize the imports and clean up some of the tests for consistency.
To be consistent with the changed unit tests,
t
was renamed to
template
and unnecessary calls tostrip()
were removed.
Diff: |
Revision 18 (+1303 -315)
|
---|

-
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
-
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.
-
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.
-
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.
-
docs/manual/extending/extensions/hooks/action-hook.rst (Diff revision 18) Two blank lines between sections.
Same below.
-
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. -
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.
-
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.)
-
-
-
reviewboard/extensions/hooks.py (Diff revision 18) This definitely need more docs. I don't have a good sense of what this does.
-
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 aKeyError
with a friendly message if one is missing. -
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', ...)
-
-
-
-
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.
-
-
reviewboard/extensions/hooks.py (Diff revision 18) Instead of setting these, just pass them inline where needed.
-
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. -
-
-
reviewboard/extensions/hooks.py (Diff revision 18) I notice we're reversing here, and then reversing again later. What's the reason for that?
-
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.
-
-
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. -
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.
-
-
-
-
reviewboard/extensions/hooks.py (Diff revision 18) This should go in an Example section, like:
""" ... Example: .. code-block:: python actions = [{ ... }]
-
reviewboard/extensions/hooks.py (Diff revision 18) Same naming thoughts as above.
Also, needs docstrings.
-
-
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.
-
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.
-
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.
-
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.
-
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. -
reviewboard/reviews/actions.py (Diff revision 18) Private attributes shouldn't be documented.
Same below.
-
reviewboard/reviews/actions.py (Diff revision 18) If a subclass is meant to override this, it should not be a private function.
-
-
-
-
reviewboard/reviews/actions.py (Diff revision 18) You can combine these to be optimistic:
try: del _all_actions[...] except KeyError: raise KeyError(...)
-
-
-
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(...)
-
-
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.)
-
-
reviewboard/reviews/default_actions.py (Diff revision 18) Blank line between these.
Though, you might want to just combine the statements.
-
reviewboard/reviews/default_actions.py (Diff revision 18) Let's have the third condition on its own line.
-
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 likeshould_render()
. -
-
reviewboard/reviews/default_actions.py (Diff revision 18) Here too.
Though, you may want to just combine the statements.
-
-
-
reviewboard/static/rb/js/extensions/models/reviewRequestActionHookModel.js (Diff revision 18) This might need to provide an example.
-
reviewboard/static/rb/js/extensions/models/reviewRequestActionHookModel.js (Diff revision 18) This should be documented.
-
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 18) If we're changing the names, let's get rid of
-link
and replace it with-action
. -
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 18) No space after
function
.
Change Summary:
Address some of the easier issues raised by Christian, like renaming.
- Tables replaced with autosummary sections (abbreviations removed).
- The versionadded sections are now aligned correctly.
- Sections now have two newlines between them.
- '-link'
action_id
suffixes have been renamed to '-action'. ONLY_REVIEW_URL_NAMES
has been moved tourls.py
.- Various docstrings have been added/improved.
- The
:py:class:
prefix was added to eachdict
. - The
_should_render
method is now public. - When using '(list of X)', the type X is now singular.
- Added the use of
:guilabel:
. - Swapped the order of
apply_to
andactions
in__init__
. - Renamed
_to_action_instance
to_convert_action
. - Unit tests now call
reset_actions()
. - Reverted the extraneous code cleanup in
tests.py
. - Removed documentation of private variables.
TODO:
- Address the remaining issues raised by Christian.
- Redesign how actions set their state at runtime.
- Add more unit tests.
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 19 (+1283 -290)
|

-
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
-
Change Summary:
Address Christian's remaining easy issues.
- Improved various docstrings, whitespace, and word choice.
- Added
ActionHook
to the list of hooks inaction-hook.rst
. - Removed confusing "...is an instance of some subclass of..." references.
- Renamed
reset_actions
->clear_all_actions
. - Removed all references to "tabs" (use "pages" instead).
- Made
DictAction
andDictMenuAction
private. - Renamed
_register_and_get
->_register_actions
and extracted out a
_normalize_action()
method. - Exceptions are raised when invalid actions are provided.
- "Example:" and "Note:" and "Warning:" sections are used.
- Extracted the
ActionHookTests
into a separate class. - Made test docstrings more specific.
- Class "Attributes:" section removed and added inline instead.
- Optimistic
try
/except
sections added. - Added an example of an extension that uses
RB.ReviewRequestActionHook
.
TODO:
- Redesign how actions set their state at runtime.
- Add more unit tests.
Diff: |
---|