Refactor the actions system
Review Request #9131 — Created Aug. 11, 2017 and discarded
This is a large patch that refactors the action registry system
implemented by a past student to use a proper registry. Much refactoring
has been done:
- Action base classes now live in
reviewboard.actions
. This package
also includes header dropdown actions. - Review request-specific action classes now live in
reviewboard.reviews.actions
. - ALL action hooks support new-style action classes and old-style
dicts. Previously, there was a discrepancy betweeen which hooks
supported which type of action. ActionHook
is now deprecated in favour ofReviewRequestActionHook
.
The behaviour was the previously the same, except ActionHook supported
old-style dicts instead of classes. Now that the behaviour of all
hooks has been unified, we no longer need this distinction.- The
{% child_actions %}
templatetag has been moved fromreviewtags
to a newactions
template tag library. It now takes the action it
should list the child actions of. - The
_DictAction
and_DictMenuAction
classes have been turned into
theDictActionMixin
. - There are now two action registries: one for review request actions
and one for header actions. This allows us to keep them seperate. - We now store instances of dict actions instead of the dicts
themselves. This is because you cannot store adict
in aset
(as
it is unhashable) and therefore cannot be put into aRegistry
. - Generic action templates now live in
templates/actions
.
- Ran unit tests.
- Built the docs and looked through them.
Description | From | Last Updated |
---|---|---|
W391 blank line at end of file |
reviewbot | |
W391 blank line at end of file |
reviewbot | |
F401 'reviewboard.actions.header.HeaderMenuAction' imported but unused |
reviewbot | |
F401 'reviewboard.actions.header.HeaderAction' imported but unused |
reviewbot | |
F841 local variable 'hook' is assigned to but never used |
reviewbot | |
F821 undefined name 'ReviewRequestAction' |
reviewbot | |
F821 undefined name 'ReviewRequestMenuAction' |
reviewbot | |
Should be changed to 4.0 now. |
chipx86 | |
Here, too. |
chipx86 | |
And here. |
chipx86 | |
= should be removed. |
chipx86 | |
Blank line between these. |
chipx86 | |
Capitalize the start of "subclasses." |
chipx86 | |
Only 1 blank line. |
chipx86 | |
Here, too. |
chipx86 | |
Here, too. |
chipx86 | |
Here, too. |
chipx86 | |
Missing docs. |
chipx86 | |
Alphabetical order. |
chipx86 | |
Missing docs. |
chipx86 | |
Missing docs. |
chipx86 | |
Missing a unicode_literals import. |
chipx86 | |
Missing docs. |
chipx86 | |
Missing the rest of the sentence. Also need one for image_height separately. |
chipx86 | |
Missing docs. |
chipx86 | |
Missing docs. |
chipx86 | |
Missing unicode_literals. |
chipx86 | |
Incomplete docstring. |
chipx86 | |
Missing period. |
chipx86 | |
Must be the full class path. |
chipx86 | |
Shouldn't use the "If ..." form. Instead, "The parent action is not registered." |
chipx86 | |
Same here. |
chipx86 | |
No trailing comma on the last parameter to a method. |
chipx86 | |
Missing docs. |
chipx86 | |
Leftover debugging. |
chipx86 | |
Missing docs. |
chipx86 | |
Must have the full class path. |
chipx86 | |
Just "The item was not foudn in the registry." |
chipx86 | |
Full class path. |
chipx86 | |
Full class path. |
chipx86 | |
Alignment issue. |
chipx86 | |
Same notes as above with the descriptions. |
chipx86 | |
Full class path. |
chipx86 | |
Description on the next line. |
chipx86 | |
We should have a module-level logger. |
chipx86 | |
Missing a file docstring. |
chipx86 | |
Missing a unicode_literals import. |
chipx86 | |
Needs a full class path. |
chipx86 | |
Description on the next line. |
chipx86 | |
Alphabetical order. |
chipx86 | |
Should be 4.0 now. And actually, this can now look like: """ ... Deprecated: 3.0: Blah blah """ |
chipx86 | |
Missing docs. |
chipx86 | |
Should use str() instead of b'' for future Python compatibility. |
chipx86 | |
Needs the full class path. |
chipx86 | |
Same here. |
chipx86 | |
And here. |
chipx86 | |
And here. |
chipx86 | |
Double repr. We don't need the explicit call. |
chipx86 | |
No double underscore on the function. That will mangle the name. Needs docs. |
chipx86 | |
Needs a full class path. |
chipx86 | |
Missing docs. |
chipx86 | |
Should use str() instead of b''. |
chipx86 | |
Needs a full class path. |
chipx86 | |
Needs a full class path. |
chipx86 | |
Needs a full class path. |
chipx86 | |
These can be on the same line. |
chipx86 | |
No double underscore. Needs docs. |
chipx86 | |
Leftover debugging. |
chipx86 | |
Missing a test docstring. |
chipx86 | |
Missing a test docstring. |
chipx86 | |
Missing a test docstring. |
chipx86 | |
Missing a test docstring. |
chipx86 | |
Missing a test docstring. |
chipx86 | |
Missing a test docstring. |
chipx86 | |
Missing a test docstring. |
chipx86 | |
Missing a test docstring. |
chipx86 | |
Missing a test docstring. |
chipx86 | |
Swap these. |
chipx86 | |
Missing docs. |
chipx86 | |
Missing docs. |
chipx86 | |
Missing docs. |
chipx86 | |
Missing docs. |
chipx86 | |
Missing docs. |
chipx86 | |
Missing docs. |
chipx86 | |
Missing docs. |
chipx86 | |
Missing docs. |
chipx86 | |
Missing docs. |
chipx86 | |
No need for staticfiles or i18n. |
chipx86 | |
There should be a book for this. |
brennie | |
E702 multiple statements on one line (semicolon) |
reviewbot |
- Description:
-
This is a large patch that refactors the action registry system
implemented by a past student to use a proper registry. Much refactoring has been done: - Action base classes now live in
reviewboard.actions
. This package
also includes header dropdown actions.
- Review request-specific action classes now live in
reviewboard.reviews.actions
.
- ALL action hooks support new-style action classes and old-style
dicts. Previously, there was a discrepancy betweeen which hooks
supported which type of action.
ActionHook
is now deprecated in favour ofReviewRequestActionHook
.
The behaviour was the previously the same, except ActionHook supported
old-style dicts instead of classes. Now that the behaviour of all
hooks has been unified, we no longer need this distinction.
- The
{% child_actions %}
templatetag has been moved fromreviewtags
to a newactions
template tag library. It now takes the action it
should list the child actions of.
- The
_DictAction
and_DictMenuAction
classes have been turned into
theDictActionMixin
.
- There are now two action registries: one for review request actions
and one for header actions. This allows us to keep them seperate.
- We now store instances of dict actions instead of the dicts
themselves. This is because you cannot store adict
in aset
(as
it is unhashable) and therefore cannot be put into aRegistry
.
- Generic action templates now live in
templates/actions
.
~ TODO: Docs
~ TODO: more unit tests.
- Action base classes now live in
- Testing Done:
-
~ Ran unit tests.
~ TODO: Smoke testing, more unit tests. ~ - Ran unit tests.
~ - Built the docs and looked through them.
- Commit:
-
add0aa18159f99869655273a52f40cbf98cda8c90ccb1bf8908fccba01c1cd83f801be0df14215da
- Diff:
-
Revision 2 (+1256 -1428)
- Added Files:
Checks run (2 succeeded)
- Change Summary:
-
Rebase
- Commit:
-
0ccb1bf8908fccba01c1cd83f801be0df14215da373472950257d0eee0be15cf79493d74a4137af3
- Diff:
-
Revision 3 (+1247 -1414)
- Change Summary:
-
Fix typo
- Commit:
-
373472950257d0eee0be15cf79493d74a4137af3a809dfe28650e474822990ae5adc13a5422f71b0
- Diff:
-
Revision 4 (+1247 -1414)
Checks run (2 succeeded)
- Change Summary:
-
Fix templates
- Commit:
-
a809dfe28650e474822990ae5adc13a5422f71b0a10e69fd25ab2807d1b0b3d8f8e8dc7d151eade7
- Diff:
-
Revision 5 (+1249 -1421)
Checks run (2 succeeded)
- Summary:
-
WIP: Refactor the actions systemRefactor the actions system
- Description:
-
This is a large patch that refactors the action registry system
implemented by a past student to use a proper registry. Much refactoring has been done: - Action base classes now live in
reviewboard.actions
. This package
also includes header dropdown actions.
- Review request-specific action classes now live in
reviewboard.reviews.actions
.
- ALL action hooks support new-style action classes and old-style
dicts. Previously, there was a discrepancy betweeen which hooks
supported which type of action.
ActionHook
is now deprecated in favour ofReviewRequestActionHook
.
The behaviour was the previously the same, except ActionHook supported
old-style dicts instead of classes. Now that the behaviour of all
hooks has been unified, we no longer need this distinction.
- The
{% child_actions %}
templatetag has been moved fromreviewtags
to a newactions
template tag library. It now takes the action it
should list the child actions of.
- The
_DictAction
and_DictMenuAction
classes have been turned into
theDictActionMixin
.
- There are now two action registries: one for review request actions
and one for header actions. This allows us to keep them seperate.
- We now store instances of dict actions instead of the dicts
themselves. This is because you cannot store adict
in aset
(as
it is unhashable) and therefore cannot be put into aRegistry
.
- Generic action templates now live in
templates/actions
.
- - TODO: more unit tests.
- Action base classes now live in
-
This is an old change at this point, and I'm in no way expecting you to finish this work. I just wanted to go through it and make suitable notes on what needs to be done before inclusion, so that we're at a good point to hand it off and continue work.
Of course you're always more than welcome to finish the change, but that's not the priority right now :)
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
- Change Summary:
-
This patch can now be applied to release-3.0.x to ease forward porting to 4.0.x.
- Commit:
-
a10e69fd25ab2807d1b0b3d8f8e8dc7d151eade7cc0e50c126f85f58d819bb5f2a2f144a03b3f749
- Diff:
-
Revision 6 (+1354 -1409)