Refactor the actions system

Review Request #9131 — Created Aug. 11, 2017 and discarded

Information

Review Board
release-3.0.x

Reviewers

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 of ReviewRequestActionHook.
    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 from reviewtags
    to a new actions template tag library. It now takes the action it
    should list the child actions of.
  • The _DictAction and _DictMenuAction classes have been turned into
    the DictActionMixin.
  • 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 a dict in a set (as
    it is unhashable) and therefore cannot be put into a Registry.
  • 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

reviewbotreviewbot

W391 blank line at end of file

reviewbotreviewbot

F401 'reviewboard.actions.header.HeaderMenuAction' imported but unused

reviewbotreviewbot

F401 'reviewboard.actions.header.HeaderAction' imported but unused

reviewbotreviewbot

F841 local variable 'hook' is assigned to but never used

reviewbotreviewbot

F821 undefined name 'ReviewRequestAction'

reviewbotreviewbot

F821 undefined name 'ReviewRequestMenuAction'

reviewbotreviewbot

Should be changed to 4.0 now.

chipx86chipx86

Here, too.

chipx86chipx86

And here.

chipx86chipx86

= should be removed.

chipx86chipx86

Blank line between these.

chipx86chipx86

Capitalize the start of "subclasses."

chipx86chipx86

Only 1 blank line.

chipx86chipx86

Here, too.

chipx86chipx86

Here, too.

chipx86chipx86

Here, too.

chipx86chipx86

Missing docs.

chipx86chipx86

Alphabetical order.

chipx86chipx86

Missing docs.

chipx86chipx86

Missing docs.

chipx86chipx86

Missing a unicode_literals import.

chipx86chipx86

Missing docs.

chipx86chipx86

Missing the rest of the sentence. Also need one for image_height separately.

chipx86chipx86

Missing docs.

chipx86chipx86

Missing docs.

chipx86chipx86

Missing unicode_literals.

chipx86chipx86

Incomplete docstring.

chipx86chipx86

Missing period.

chipx86chipx86

Must be the full class path.

chipx86chipx86

Shouldn't use the "If ..." form. Instead, "The parent action is not registered."

chipx86chipx86

Same here.

chipx86chipx86

No trailing comma on the last parameter to a method.

chipx86chipx86

Missing docs.

chipx86chipx86

Leftover debugging.

chipx86chipx86

Missing docs.

chipx86chipx86

Must have the full class path.

chipx86chipx86

Just "The item was not foudn in the registry."

chipx86chipx86

Full class path.

chipx86chipx86

Full class path.

chipx86chipx86

Alignment issue.

chipx86chipx86

Same notes as above with the descriptions.

chipx86chipx86

Full class path.

chipx86chipx86

Description on the next line.

chipx86chipx86

We should have a module-level logger.

chipx86chipx86

Missing a file docstring.

chipx86chipx86

Missing a unicode_literals import.

chipx86chipx86

Needs a full class path.

chipx86chipx86

Description on the next line.

chipx86chipx86

Alphabetical order.

chipx86chipx86

Should be 4.0 now. And actually, this can now look like: """ ... Deprecated: 3.0: Blah blah """

chipx86chipx86

Missing docs.

chipx86chipx86

Should use str() instead of b'' for future Python compatibility.

chipx86chipx86

Needs the full class path.

chipx86chipx86

Same here.

chipx86chipx86

And here.

chipx86chipx86

And here.

chipx86chipx86

Double repr. We don't need the explicit call.

chipx86chipx86

No double underscore on the function. That will mangle the name. Needs docs.

chipx86chipx86

Needs a full class path.

chipx86chipx86

Missing docs.

chipx86chipx86

Should use str() instead of b''.

chipx86chipx86

Needs a full class path.

chipx86chipx86

Needs a full class path.

chipx86chipx86

Needs a full class path.

chipx86chipx86

These can be on the same line.

chipx86chipx86

No double underscore. Needs docs.

chipx86chipx86

Leftover debugging.

chipx86chipx86

Missing a test docstring.

chipx86chipx86

Missing a test docstring.

chipx86chipx86

Missing a test docstring.

chipx86chipx86

Missing a test docstring.

chipx86chipx86

Missing a test docstring.

chipx86chipx86

Missing a test docstring.

chipx86chipx86

Missing a test docstring.

chipx86chipx86

Missing a test docstring.

chipx86chipx86

Missing a test docstring.

chipx86chipx86

Swap these.

chipx86chipx86

Missing docs.

chipx86chipx86

Missing docs.

chipx86chipx86

Missing docs.

chipx86chipx86

Missing docs.

chipx86chipx86

Missing docs.

chipx86chipx86

Missing docs.

chipx86chipx86

Missing docs.

chipx86chipx86

Missing docs.

chipx86chipx86

Missing docs.

chipx86chipx86

No need for staticfiles or i18n.

chipx86chipx86

There should be a book for this.

brenniebrennie

E702 multiple statements on one line (semicolon)

reviewbotreviewbot
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

brennie
brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
brennie
chipx86
  1. 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 :)

    1. This patch applied cleanly to e794a0945dcc1ed16d5bc2321c5eadc33a1374cc. It does not apply cleanly to 3.0.x

  2. Show all issues

    Should be changed to 4.0 now.

  3. Show all issues

    Here, too.

  4. Show all issues

    And here.

  5. reviewboard/actions/base.py (Diff revision 5)
     
     
    Show all issues

    = should be removed.

  6. reviewboard/actions/base.py (Diff revision 5)
     
     
     
    Show all issues

    Blank line between these.

  7. reviewboard/actions/base.py (Diff revision 5)
     
     
    Show all issues

    Capitalize the start of "subclasses."

  8. reviewboard/actions/base.py (Diff revision 5)
     
     
     
     
     
    Show all issues

    Only 1 blank line.

  9. reviewboard/actions/base.py (Diff revision 5)
     
     
    Show all issues

    Here, too.

  10. reviewboard/actions/base.py (Diff revision 5)
     
     
    Show all issues

    Here, too.

  11. reviewboard/actions/base.py (Diff revision 5)
     
     
    Show all issues

    Here, too.

  12. reviewboard/actions/base.py (Diff revision 5)
     
     
    Show all issues

    Missing docs.

  13. reviewboard/actions/base.py (Diff revision 5)
     
     
     
     
     
    Show all issues

    Alphabetical order.

  14. reviewboard/actions/base.py (Diff revision 5)
     
     
     
    Show all issues

    Missing docs.

  15. reviewboard/actions/base.py (Diff revision 5)
     
     
    Show all issues

    Missing docs.

  16. reviewboard/actions/header.py (Diff revision 5)
     
     
     
     
    Show all issues

    Missing a unicode_literals import.

  17. reviewboard/actions/header.py (Diff revision 5)
     
     
    Show all issues

    Missing docs.

  18. reviewboard/actions/header.py (Diff revision 5)
     
     
    Show all issues

    Missing the rest of the sentence.

    Also need one for image_height separately.

  19. reviewboard/actions/header.py (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Missing docs.

  20. reviewboard/actions/header.py (Diff revision 5)
     
     
    Show all issues

    Missing docs.

  21. reviewboard/actions/registry.py (Diff revision 5)
     
     
     
     
    Show all issues

    Missing unicode_literals.

  22. reviewboard/actions/registry.py (Diff revision 5)
     
     
    Show all issues

    Incomplete docstring.

  23. reviewboard/actions/registry.py (Diff revision 5)
     
     
    Show all issues

    Missing period.

  24. reviewboard/actions/registry.py (Diff revision 5)
     
     
    Show all issues

    Must be the full class path.

  25. reviewboard/actions/registry.py (Diff revision 5)
     
     
    Show all issues

    Shouldn't use the "If ..." form. Instead, "The parent action is not registered."

  26. reviewboard/actions/registry.py (Diff revision 5)
     
     
    Show all issues

    Same here.

  27. reviewboard/actions/registry.py (Diff revision 5)
     
     
     
    Show all issues

    No trailing comma on the last parameter to a method.

  28. reviewboard/actions/registry.py (Diff revision 5)
     
     
    Show all issues

    Missing docs.

  29. reviewboard/actions/registry.py (Diff revision 5)
     
     
     
     
     
     
    Show all issues

    Leftover debugging.

  30. reviewboard/actions/registry.py (Diff revision 5)
     
     
    Show all issues

    Missing docs.

  31. reviewboard/actions/registry.py (Diff revision 5)
     
     
    Show all issues

    Must have the full class path.

  32. reviewboard/actions/registry.py (Diff revision 5)
     
     
    Show all issues

    Just "The item was not foudn in the registry."

  33. reviewboard/actions/registry.py (Diff revision 5)
     
     
    Show all issues

    Full class path.

  34. reviewboard/actions/registry.py (Diff revision 5)
     
     
    Show all issues

    Full class path.

  35. reviewboard/actions/registry.py (Diff revision 5)
     
     
     
    Show all issues

    Alignment issue.

  36. reviewboard/actions/registry.py (Diff revision 5)
     
     
     
     
     
     
    Show all issues

    Same notes as above with the descriptions.

  37. reviewboard/actions/registry.py (Diff revision 5)
     
     
    Show all issues

    Full class path.

  38. Show all issues

    Description on the next line.

  39. reviewboard/actions/templatetags/actions.py (Diff revision 5)
     
     
     
    Show all issues

    We should have a module-level logger.

  40. reviewboard/actions/tests.py (Diff revision 5)
     
     
    Show all issues

    Missing a file docstring.

  41. reviewboard/extensions/actions.py (Diff revision 5)
     
     
    Show all issues

    Missing a unicode_literals import.

  42. reviewboard/extensions/actions.py (Diff revision 5)
     
     
    Show all issues

    Needs a full class path.

  43. reviewboard/extensions/actions.py (Diff revision 5)
     
     
    Show all issues

    Description on the next line.

  44. reviewboard/extensions/hooks.py (Diff revision 5)
     
     
     
    Show all issues

    Alphabetical order.

  45. reviewboard/extensions/hooks.py (Diff revision 5)
     
     
    Show all issues

    Should be 4.0 now.

    And actually, this can now look like:

    """
    ...
    
    Deprecated:
        3.0:
        Blah blah
    """
    
  46. reviewboard/extensions/hooks.py (Diff revision 5)
     
     
    Show all issues

    Missing docs.

  47. reviewboard/extensions/hooks.py (Diff revision 5)
     
     
    Show all issues

    Should use str() instead of b'' for future Python compatibility.

  48. reviewboard/extensions/hooks.py (Diff revision 5)
     
     
    Show all issues

    Needs the full class path.

  49. reviewboard/extensions/hooks.py (Diff revision 5)
     
     
    Show all issues

    Same here.

  50. reviewboard/extensions/hooks.py (Diff revision 5)
     
     
    Show all issues

    And here.

  51. reviewboard/extensions/hooks.py (Diff revision 5)
     
     
    Show all issues

    And here.

  52. reviewboard/extensions/hooks.py (Diff revision 5)
     
     
     
    Show all issues

    Double repr. We don't need the explicit call.

  53. reviewboard/extensions/hooks.py (Diff revision 5)
     
     
    Show all issues

    No double underscore on the function. That will mangle the name.

    Needs docs.

    1. This was intentional.

    2. Or at least it was at some point. I recall there being multiple __from_dict_action methods but :shrug:

  54. reviewboard/extensions/hooks.py (Diff revision 5)
     
     
    Show all issues

    Needs a full class path.

  55. reviewboard/extensions/hooks.py (Diff revision 5)
     
     
    Show all issues

    Missing docs.

  56. reviewboard/extensions/hooks.py (Diff revision 5)
     
     
    Show all issues

    Should use str() instead of b''.

  57. reviewboard/extensions/hooks.py (Diff revision 5)
     
     
    Show all issues

    Needs a full class path.

  58. reviewboard/extensions/hooks.py (Diff revision 5)
     
     
     
     
    Show all issues

    Needs a full class path.

  59. reviewboard/extensions/hooks.py (Diff revision 5)
     
     
    Show all issues

    Needs a full class path.

  60. reviewboard/extensions/hooks.py (Diff revision 5)
     
     
     
    Show all issues

    These can be on the same line.

  61. reviewboard/extensions/hooks.py (Diff revision 5)
     
     
    Show all issues

    No double underscore.

    Needs docs.

  62. Show all issues

    Leftover debugging.

  63. reviewboard/extensions/tests.py (Diff revision 5)
     
     
    Show all issues

    Missing a test docstring.

  64. reviewboard/extensions/tests.py (Diff revision 5)
     
     
    Show all issues

    Missing a test docstring.

  65. reviewboard/extensions/tests.py (Diff revision 5)
     
     
    Show all issues

    Missing a test docstring.

  66. reviewboard/extensions/tests.py (Diff revision 5)
     
     
    Show all issues

    Missing a test docstring.

  67. reviewboard/extensions/tests.py (Diff revision 5)
     
     
    Show all issues

    Missing a test docstring.

  68. reviewboard/extensions/tests.py (Diff revision 5)
     
     
    Show all issues

    Missing a test docstring.

  69. reviewboard/extensions/tests.py (Diff revision 5)
     
     
    Show all issues

    Missing a test docstring.

  70. reviewboard/extensions/tests.py (Diff revision 5)
     
     
    Show all issues

    Missing a test docstring.

  71. reviewboard/extensions/tests.py (Diff revision 5)
     
     
    Show all issues

    Missing a test docstring.

  72. reviewboard/reviews/actions.py (Diff revision 5)
     
     
     
    Show all issues

    Swap these.

  73. reviewboard/reviews/actions.py (Diff revision 5)
     
     
    Show all issues

    Missing docs.

  74. reviewboard/reviews/actions.py (Diff revision 5)
     
     
    Show all issues

    Missing docs.

  75. reviewboard/reviews/actions.py (Diff revision 5)
     
     
    Show all issues

    Missing docs.

  76. reviewboard/reviews/actions.py (Diff revision 5)
     
     
    Show all issues

    Missing docs.

  77. reviewboard/reviews/actions.py (Diff revision 5)
     
     
    Show all issues

    Missing docs.

  78. reviewboard/reviews/actions.py (Diff revision 5)
     
     
    Show all issues

    Missing docs.

  79. reviewboard/reviews/actions.py (Diff revision 5)
     
     
    Show all issues

    Missing docs.

  80. reviewboard/reviews/actions.py (Diff revision 5)
     
     
    Show all issues

    Missing docs.

  81. reviewboard/reviews/actions.py (Diff revision 5)
     
     
    Show all issues

    Missing docs.

  82. Show all issues

    No need for staticfiles or i18n.

  83. 
      
brennie
Review request changed

Change Summary:

This patch can now be applied to release-3.0.x to ease forward porting to 4.0.x.

Commit:

-a10e69fd25ab2807d1b0b3d8f8e8dc7d151eade7
+cc0e50c126f85f58d819bb5f2a2f144a03b3f749

Diff:

Revision 6 (+1354 -1409)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
  1. 
      
  2. Show all issues

    There should be a book for this.

  3. 
      
david
Review request changed

Status: Discarded

Loading...