Refactor the actions system

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

brennie
Review Board
release-3.0.x
a10e69f...
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.
Loading file attachments...

  • 81
  • 0
  • 7
  • 0
  • 88
Description From Last Updated
Should be changed to 4.0 now. chipx86 chipx86
Here, too. chipx86 chipx86
And here. chipx86 chipx86
= should be removed. chipx86 chipx86
Blank line between these. chipx86 chipx86
Capitalize the start of "subclasses." chipx86 chipx86
Only 1 blank line. chipx86 chipx86
Here, too. chipx86 chipx86
Here, too. chipx86 chipx86
Here, too. chipx86 chipx86
Missing docs. chipx86 chipx86
Alphabetical order. chipx86 chipx86
Missing docs. chipx86 chipx86
Missing docs. chipx86 chipx86
Missing a unicode_literals import. chipx86 chipx86
Missing docs. chipx86 chipx86
Missing the rest of the sentence. Also need one for image_height separately. chipx86 chipx86
Missing docs. chipx86 chipx86
Missing docs. chipx86 chipx86
Missing unicode_literals. chipx86 chipx86
Incomplete docstring. chipx86 chipx86
Missing period. chipx86 chipx86
Must be the full class path. chipx86 chipx86
Shouldn't use the "If ..." form. Instead, "The parent action is not registered." chipx86 chipx86
Same here. chipx86 chipx86
No trailing comma on the last parameter to a method. chipx86 chipx86
Missing docs. chipx86 chipx86
Leftover debugging. chipx86 chipx86
Missing docs. chipx86 chipx86
Must have the full class path. chipx86 chipx86
Just "The item was not foudn in the registry." chipx86 chipx86
Full class path. chipx86 chipx86
Full class path. chipx86 chipx86
Alignment issue. chipx86 chipx86
Same notes as above with the descriptions. chipx86 chipx86
Full class path. chipx86 chipx86
Description on the next line. chipx86 chipx86
We should have a module-level logger. chipx86 chipx86
Missing a file docstring. chipx86 chipx86
Missing a unicode_literals import. chipx86 chipx86
Needs a full class path. chipx86 chipx86
Description on the next line. chipx86 chipx86
Alphabetical order. chipx86 chipx86
Should be 4.0 now. And actually, this can now look like: """ ... Deprecated: 3.0: Blah blah """ chipx86 chipx86
Missing docs. chipx86 chipx86
Should use str() instead of b'' for future Python compatibility. chipx86 chipx86
Needs the full class path. chipx86 chipx86
Same here. chipx86 chipx86
And here. chipx86 chipx86
And here. chipx86 chipx86
Double repr. We don't need the explicit call. chipx86 chipx86
No double underscore on the function. That will mangle the name. Needs docs. chipx86 chipx86
Needs a full class path. chipx86 chipx86
Missing docs. chipx86 chipx86
Should use str() instead of b''. chipx86 chipx86
Needs a full class path. chipx86 chipx86
Needs a full class path. chipx86 chipx86
Needs a full class path. chipx86 chipx86
These can be on the same line. chipx86 chipx86
No double underscore. Needs docs. chipx86 chipx86
Leftover debugging. chipx86 chipx86
Missing a test docstring. chipx86 chipx86
Missing a test docstring. chipx86 chipx86
Missing a test docstring. chipx86 chipx86
Missing a test docstring. chipx86 chipx86
Missing a test docstring. chipx86 chipx86
Missing a test docstring. chipx86 chipx86
Missing a test docstring. chipx86 chipx86
Missing a test docstring. chipx86 chipx86
Missing a test docstring. chipx86 chipx86
Swap these. chipx86 chipx86
Missing docs. chipx86 chipx86
Missing docs. chipx86 chipx86
Missing docs. chipx86 chipx86
Missing docs. chipx86 chipx86
Missing docs. chipx86 chipx86
Missing docs. chipx86 chipx86
Missing docs. chipx86 chipx86
Missing docs. chipx86 chipx86
Missing docs. chipx86 chipx86
No need for staticfiles or i18n. chipx86 chipx86
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
Review request changed

Summary:

-WIP: Refactor the actions system
+Refactor 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 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.
-  
-  

TODO: more unit tests.

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.

  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. 
      
Loading...