Refactor the actions system

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

brennie
Review Board
release-3.0.x
reviewboard

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. Should be changed to 4.0 now.

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

    = should be removed.

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

    Blank line between these.

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

    Capitalize the start of "subclasses."

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

    Only 1 blank line.

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

    Here, too.

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

    Here, too.

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

    Here, too.

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

    Missing docs.

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

    Alphabetical order.

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

    Missing docs.

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

    Missing docs.

  14. reviewboard/actions/header.py (Diff revision 5)
     
     
     
     

    Missing a unicode_literals import.

  15. reviewboard/actions/header.py (Diff revision 5)
     
     

    Missing docs.

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

    Missing the rest of the sentence.

    Also need one for image_height separately.

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

    Missing docs.

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

    Missing docs.

  19. reviewboard/actions/registry.py (Diff revision 5)
     
     
     
     

    Missing unicode_literals.

  20. reviewboard/actions/registry.py (Diff revision 5)
     
     

    Incomplete docstring.

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

    Missing period.

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

    Must be the full class path.

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

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

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

    Same here.

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

    No trailing comma on the last parameter to a method.

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

    Missing docs.

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

    Leftover debugging.

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

    Missing docs.

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

    Must have the full class path.

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

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

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

    Full class path.

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

    Full class path.

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

    Alignment issue.

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

    Same notes as above with the descriptions.

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

    Full class path.

  36. Description on the next line.

  37. reviewboard/actions/templatetags/actions.py (Diff revision 5)
     
     
     

    We should have a module-level logger.

  38. reviewboard/actions/tests.py (Diff revision 5)
     
     

    Missing a file docstring.

  39. reviewboard/extensions/actions.py (Diff revision 5)
     
     

    Missing a unicode_literals import.

  40. reviewboard/extensions/actions.py (Diff revision 5)
     
     

    Needs a full class path.

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

    Description on the next line.

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

    Alphabetical order.

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

    Should be 4.0 now.

    And actually, this can now look like:

    """
    ...
    
    Deprecated:
        3.0:
        Blah blah
    """
    
  44. reviewboard/extensions/hooks.py (Diff revision 5)
     
     

    Missing docs.

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

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

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

    Needs the full class path.

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

    Same here.

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

    And here.

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

    And here.

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

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

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

    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:

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

    Needs a full class path.

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

    Missing docs.

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

    Should use str() instead of b''.

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

    Needs a full class path.

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

    Needs a full class path.

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

    Needs a full class path.

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

    These can be on the same line.

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

    No double underscore.

    Needs docs.

  60. Leftover debugging.

  61. reviewboard/extensions/tests.py (Diff revision 5)
     
     

    Missing a test docstring.

  62. reviewboard/extensions/tests.py (Diff revision 5)
     
     

    Missing a test docstring.

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

    Missing a test docstring.

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

    Missing a test docstring.

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

    Missing a test docstring.

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

    Missing a test docstring.

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

    Missing a test docstring.

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

    Missing a test docstring.

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

    Missing a test docstring.

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

    Swap these.

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

    Missing docs.

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

    Missing docs.

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

    Missing docs.

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

    Missing docs.

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

    Missing docs.

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

    Missing docs.

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

    Missing docs.

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

    Missing docs.

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

    Missing docs.

  80. No need for staticfiles or i18n.

  81. 
      
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. There should be a book for this.

  3. 
      
david
Review request changed

Status: Discarded

Loading...