flake8
failed.
JSHint
passed.
flake8
-
reviewboard/extensions/tests.py (Diff revision 1)
Feature-gate the new class-based actions system and shutdown hooks correctly
Review Request #9384 — Created Nov. 17, 2017 and submitted
Since https://reviews.reviewboard.org/r/9131/ did not make it into the
release window for RB 3.0, we cannot address the issues in the original
implementation of class-based actions. So we are gating them behind a
feature so they can be marked as experimental (and not to be used in
production). That way we will be able to fix up the issues in the
implementation for a future release.This patch also addresses the issue of the new action hooks not
unregistering their child actions.Documentation on action hooks has been reverted to how it was in
Review Board 2.5.
- Unit tests all pass.
- Wrote a test extension which (1) registered a hook that uses
class-based actions and (2) registered a hook using dict-based
actions. The former were not available (and was logged as such) and
the latter were.
| Description | From | Last Updated |
|---|---|---|
|
E501 line too long (80 > 79 characters) |
 reviewbot
|
|
|
I don't think we need the conditional here. In the only case where uses_experimental_actions is True, self.actions is set to … |
 david
|
|
|
I don't think "Class-Based" needs to be capitalized here. |
david
|
|
|
This can remain action-hooks, since that anchor is valid for the "new" (as of this change) docs. |
 chipx86
|
|
|
This can also now be simplified into a single level: if (not class_based_actions_feature.is_enabled() and not all(isinstance(action, dict) for action in … |
david
|
|
|
Let's also state that the API is expected to change in coming patch releases. |
chipx86
|
|
|
Blank line between these. |
chipx86
|
|
|
This should be toward the end of the class. If we also use self.__class__.__name__, we don't need to override this … |
chipx86
|
|
|
E211 whitespace before '(' |
reviewbot
|
Change Summary:
Revert docs. PEP8 fixup.
Description: |
|
||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||||||||||||||||||||||||||
Diff: |
Revision 2 (+249 -279) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
reviewboard/extensions/hooks.py (Diff revision 2) I don't think we need the conditional here. In the only case where
uses_experimental_actionsisTrue,self.actionsis set to[](and so calling_register_actionswill be a no-op). -
reviewboard/reviews/features.py (Diff revision 2) I don't think "Class-Based" needs to be capitalized here.
Change Summary:
Addressed David's issues
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+248 -279) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 4 (+246 -279) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
reviewboard/extensions/hooks.py (Diff revision 4) This can also now be simplified into a single level:
if (not class_based_actions_feature.is_enabled() and not all(isinstance(action, dict) for action in actions)): logging.error(...) actions = []
-
-
docs/manual/extending/index.rst (Diff revision 4) This can remain
action-hooks, since that anchor is valid for the "new" (as of this change) docs. -
reviewboard/extensions/hooks.py (Diff revision 4) Let's also state that the API is expected to change in coming patch releases.
-
-
reviewboard/reviews/actions.py (Diff revision 4) This should be toward the end of the class.
If we also use
self.__class__.__name__, we don't need to override this for subclasses.
Change Summary:
Addressed David & Christian's issues
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 5 (+235 -277) |
Checks run (1 failed, 1 succeeded)
flake8
failed.
JSHint
passed.
flake8
Change Summary:
Formatting
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 6 (+235 -277) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.