Add base classes and registry for new actions framework.

Review Request #12752 — Created Dec. 7, 2022 and submitted

david
Review Board
release-6.x
reviewboard

The actions framework is in the process of being rewritten to give us
better consistency across all the different types of actions, a real
registry, and new features and capabilities.

This change introduces the actions app with the action base classes and
registry, along with a JavaScript-side stub model/view, template tags
for rendering the actions on pages, and the beginnings of some unit
tests.

  • Ran unit tests.
  • Used this with other changes that start porting existing actions over
    to the new framework.
Summary
Add base classes and registry for new actions framework.
Description From Last Updated

Expected a string and instead saw %. Column: 2 Error code: W095

reviewbotreviewbot

Expected ':' and instead saw 'load'. Column: 4 Error code: E021

reviewbotreviewbot

Expected a JSON value. Column: 9 Error code: E003

reviewbotreviewbot

Expected '}' and instead saw 'djblets_js'. Column: 9 Error code: E021

reviewbotreviewbot

Unrecoverable syntax error. (8% scanned). Column: 9 Error code: E041

reviewbotreviewbot

Expected a string and instead saw %. Column: 2 Error code: W095

reviewbotreviewbot

Expected ':' and instead saw 'load'. Column: 4 Error code: E021

reviewbotreviewbot

Expected a JSON value. Column: 9 Error code: E003

reviewbotreviewbot

Expected '}' and instead saw 'djblets_js'. Column: 9 Error code: E021

reviewbotreviewbot

Unrecoverable syntax error. (8% scanned). Column: 9 Error code: E041

reviewbotreviewbot

If an extension wants to provide its own action points, building on top of our action system, I think the …

chipx86chipx86

Rather than hidden, can we inverse this and use visible? That would pair better with things like fields/fieldsets.

chipx86chipx86

Can we add an "Instance variables" section above and document the types + doc comments for these in that?

chipx86chipx86

This is missing a Type:

chipx86chipx86

Second return will never be invoked.

chipx86chipx86

Can we alphabetize these?

chipx86chipx86

This feels very implementation-specific. Is there a more general name we could use to convey what this ultimately does?

chipx86chipx86

Missing a docstring. We should also document the attributes in the class body in an "Instance variables" section.

chipx86chipx86

Plain imports go before froms.

chipx86chipx86

Missing trailing comma.

chipx86chipx86

Can we separate this with a blank line, so it doesn't blend together?

chipx86chipx86

This needs to be the full class path. Same below.

chipx86chipx86

Let's sort these in alphabetical order.

chipx86chipx86

Can we break once found?

chipx86chipx86

Let's make this keyword-only.

chipx86chipx86

Can we include the exception error message in this string? I know there's a traceback but it helps when grepping …

chipx86chipx86

Can we add Model Attributes:? Same for other JavaScript models.

chipx86chipx86

Blank line here.

chipx86chipx86

Expected a string and instead saw %. Column: 2 Error code: W095

reviewbotreviewbot

Expected ':' and instead saw 'load'. Column: 4 Error code: E021

reviewbotreviewbot

Expected a JSON value. Column: 9 Error code: E003

reviewbotreviewbot

Expected '}' and instead saw 'djblets_js'. Column: 9 Error code: E021

reviewbotreviewbot

Unrecoverable syntax error. (8% scanned). Column: 9 Error code: E041

reviewbotreviewbot

The first parameter can probably just use |json_dumps without the other {}, since it's not merging it in with anything.

chipx86chipx86

Expected ':' and instead saw 'load'. Column: 4 Error code: E021

reviewbotreviewbot

Expected a JSON value. Column: 9 Error code: E003

reviewbotreviewbot

Expected '}' and instead saw 'djblets_js'. Column: 9 Error code: E021

reviewbotreviewbot

Unrecoverable syntax error. (10% scanned). Column: 9 Error code: E041

reviewbotreviewbot

Expected a string and instead saw %. Column: 2 Error code: W095

reviewbotreviewbot

Expected a string and instead saw %. Column: 2 Error code: W095

reviewbotreviewbot

Expected ':' and instead saw 'load'. Column: 4 Error code: E021

reviewbotreviewbot

Expected a JSON value. Column: 9 Error code: E003

reviewbotreviewbot

Expected '}' and instead saw 'djblets_js'. Column: 9 Error code: E021

reviewbotreviewbot

Unrecoverable syntax error. (10% scanned). Column: 9 Error code: E041

reviewbotreviewbot

Missing a period.

maubinmaubin

I think it'd be good to add some docstrings to these explaining what each attachment point is, like what it …

maubinmaubin

Should we include the exception details in the log?

maubinmaubin

Should we include the exception details in the log?

maubinmaubin

Could add -> None to the rest of these test methods.

maubinmaubin

Expected a string and instead saw %. Column: 2 Error code: W095

reviewbotreviewbot

Expected ':' and instead saw 'load'. Column: 4 Error code: E021

reviewbotreviewbot

Expected a JSON value. Column: 9 Error code: E003

reviewbotreviewbot

Expected '}' and instead saw 'djblets_js'. Column: 9 Error code: E021

reviewbotreviewbot

Unrecoverable syntax error. (10% scanned). Column: 9 Error code: E041

reviewbotreviewbot

Expected ':' and instead saw 'load'. Column: 4 Error code: E021

reviewbotreviewbot

Expected a string and instead saw %. Column: 2 Error code: W095

reviewbotreviewbot

Expected a JSON value. Column: 9 Error code: E003

reviewbotreviewbot

Expected '}' and instead saw 'djblets_js'. Column: 9 Error code: E021

reviewbotreviewbot

Unrecoverable syntax error. (10% scanned). Column: 9 Error code: E041

reviewbotreviewbot

Expected a string and instead saw %. Column: 2 Error code: W095

reviewbotreviewbot

Expected ':' and instead saw 'load'. Column: 4 Error code: E021

reviewbotreviewbot

Expected a JSON value. Column: 9 Error code: E003

reviewbotreviewbot

Expected '}' and instead saw 'djblets_js'. Column: 9 Error code: E021

reviewbotreviewbot

Unrecoverable syntax error. (10% scanned). Column: 9 Error code: E041

reviewbotreviewbot

Since these are documented as instance variables, we no longer need to have these here.

chipx86chipx86

Pure nit, but maybe alphabetize these, so it's clear where new entries should go?

chipx86chipx86

For most of these functions, we can err on the side of making things like context and maybe request keyword-only …

chipx86chipx86

Can we order these alphabetically?

chipx86chipx86

Can we make at least depth_limit a keyword-only argument?

chipx86chipx86

Do we want to localize this string?

chipx86chipx86

Descriptions are a little inconsistent. We can probably omit the "Raised" parts.

chipx86chipx86

Can we add an explicit typing for parent at the top of the function? I've seen either mypy or pyright …

chipx86chipx86

Mind adding a comment with what you were telling me during my previous review, so it's clear that there may …

chipx86chipx86

This can use yield from.

chipx86chipx86

We may need to type this.

chipx86chipx86

We may need to type this.

chipx86chipx86

We may need to type this.

chipx86chipx86

We should probably document our interfaces and their types. I don't know what the right thing is for then documenting …

chipx86chipx86

Can we have a doc comment for the interface?

chipx86chipx86

"ID"

chipx86chipx86

I almost left an entirely different comment, but now I realize Action is meant to be a generic. Might be …

chipx86chipx86

I've kind of been adopting Python's import groups rules. I think that's a nice way of keeping things a bit …

chipx86chipx86

Can we have a doc comment for the interface?

chipx86chipx86

What's this for?

chipx86chipx86

Every time .render() is called, we'll get a new menu appended. I'm assuming we don't want that? Should this be …

chipx86chipx86

When RB.MenuView is converted, we'll be able to do: this.menu.renderInto(this.el);

chipx86chipx86

Do we want these to be private?

chipx86chipx86

A switch might be a better choice here for all these. I think JavaScript can optimize that better too.

chipx86chipx86

Alphabetical order? Just to keep it manageable.

chipx86chipx86

Expected a string and instead saw %. Column: 2 Error code: W095

reviewbotreviewbot

Expected ':' and instead saw 'load'. Column: 4 Error code: E021

reviewbotreviewbot

Expected a JSON value. Column: 9 Error code: E003

reviewbotreviewbot

Expected '}' and instead saw 'djblets_js'. Column: 9 Error code: E021

reviewbotreviewbot

Unrecoverable syntax error. (10% scanned). Column: 9 Error code: E041

reviewbotreviewbot

Expected '}' and instead saw 'djblets_js'. Column: 9 Error code: E021

reviewbotreviewbot

Unrecoverable syntax error. (10% scanned). Column: 9 Error code: E041

reviewbotreviewbot

Expected a JSON value. Column: 9 Error code: E003

reviewbotreviewbot

Expected ':' and instead saw 'load'. Column: 4 Error code: E021

reviewbotreviewbot

Expected a string and instead saw %. Column: 2 Error code: W095

reviewbotreviewbot

Expected a string and instead saw %. Column: 2 Error code: W095

reviewbotreviewbot

Expected ':' and instead saw 'load'. Column: 4 Error code: E021

reviewbotreviewbot

Expected a JSON value. Column: 9 Error code: E003

reviewbotreviewbot

Expected '}' and instead saw 'djblets_js'. Column: 9 Error code: E021

reviewbotreviewbot

Unrecoverable syntax error. (10% scanned). Column: 9 Error code: E041

reviewbotreviewbot

Expected a string and instead saw %. Column: 2 Error code: W095

reviewbotreviewbot

Expected ':' and instead saw 'load'. Column: 4 Error code: E021

reviewbotreviewbot

Expected a JSON value. Column: 9 Error code: E003

reviewbotreviewbot

Expected '}' and instead saw 'djblets_js'. Column: 9 Error code: E021

reviewbotreviewbot

Unrecoverable syntax error. (10% scanned). Column: 9 Error code: E041

reviewbotreviewbot

Can we add a Version Added here, and a Version Changed on the function description to document it?

chipx86chipx86

Expected a string and instead saw %. Column: 2 Error code: W095

reviewbotreviewbot

Expected ':' and instead saw 'load'. Column: 4 Error code: E021

reviewbotreviewbot

Expected a JSON value. Column: 9 Error code: E003

reviewbotreviewbot

Expected '}' and instead saw 'djblets_js'. Column: 9 Error code: E021

reviewbotreviewbot

Unrecoverable syntax error. (10% scanned). Column: 9 Error code: E041

reviewbotreviewbot

Let's go with import groups and do a blank line between these.

chipx86chipx86

Expected '}' and instead saw 'djblets_js'. Column: 9 Error code: E021

reviewbotreviewbot

Unrecoverable syntax error. (10% scanned). Column: 9 Error code: E041

reviewbotreviewbot

Expected a JSON value. Column: 9 Error code: E003

reviewbotreviewbot

Expected ':' and instead saw 'load'. Column: 4 Error code: E021

reviewbotreviewbot

Expected a string and instead saw %. Column: 2 Error code: W095

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

JSHint

david
Review request changed

Change Summary:

Add some more pieces that got missed in the initial commit.

Commits:

Summary
-
Add base classes and registry for new actions framework.
+
Add base classes and registry for new actions framework.

Diff:

Revision 2 (+1688)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

david
Review request changed

Change Summary:

  • Implement apply_to
  • Add maximum depth checking for nested items.
  • Change it so registering nested items out of order (child first, then parent) will be properly handled with a warning rather than causing an assertion error.
  • Add missing menu action JS view.

Commits:

Summary
-
Add base classes and registry for new actions framework.
+
Add base classes and registry for new actions framework.

Diff:

Revision 3 (+2238)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

chipx86
  1. 
      
  2. reviewboard/actions/base.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     

    If an extension wants to provide its own action points, building on top of our action system, I think the enum would prevent that (from a typing perspective), right?

    Maybe just take arbitrary strings as attachment points and have an object of constants for our built-in ones?

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

    Rather than hidden, can we inverse this and use visible? That would pair better with things like fields/fieldsets.

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

    Can we add an "Instance variables" section above and document the types + doc comments for these in that?

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

    This is missing a Type:

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

    Second return will never be invoked.

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

    So I've noticed that our general defaults of None for class attributes has been a bit annoying when working with those values, because we always need to do these checks.

    That might still be the right approach. Another, though, would be to default those to empty strings instead of None, and then simply return or assert for truthiness, depending on where we're using it.

    In simple cases like this, not a big deal, but it's definitely made porting some older code a bit more of a hassle.

    I'd love to figure out what the right pattern is going forward.

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

    Can we alphabetize these?

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

    This feels very implementation-specific. Is there a more general name we could use to convey what this ultimately does?

  10. reviewboard/actions/errors.py (Diff revision 3)
     
     
     
     
     

    I always forget the particulars here: Two lines or one line when there's no imports?

  11. reviewboard/actions/errors.py (Diff revision 3)
     
     
     
     
     
     

    Missing a docstring.

    We should also document the attributes in the class body in an "Instance variables" section.

  12. reviewboard/actions/registry.py (Diff revision 3)
     
     
     

    Plain imports go before froms.

  13. reviewboard/actions/registry.py (Diff revision 3)
     
     

    Missing trailing comma.

  14. reviewboard/actions/registry.py (Diff revision 3)
     
     
     

    Can we separate this with a blank line, so it doesn't blend together?

  15. reviewboard/actions/registry.py (Diff revision 3)
     
     

    This needs to be the full class path.

    Same below.

  16. reviewboard/actions/registry.py (Diff revision 3)
     
     
     
     
     
     
     
     

    Let's sort these in alphabetical order.

  17. reviewboard/actions/registry.py (Diff revision 3)
     
     
     
     
     

    Can we break once found?

    1. There isn't a limit to the number of deferred registrations. If someone has written their stuff out of order, they might register a whole bunch of child actions before the parent.

  18. reviewboard/actions/registry.py (Diff revision 3)
     
     

    Let's make this keyword-only.

  19. reviewboard/actions/templatetags/actions.py (Diff revision 3)
     
     
     
     

    Can we include the exception error message in this string? I know there's a traceback but it helps when grepping for errors.

    Same below.

  20. reviewboard/static/rb/js/actions/models/actionModel.ts (Diff revision 3)
     
     
     
     
     
     
     
     

    Can we add Model Attributes:?

    Same for other JavaScript models.

  21. reviewboard/templates/actions/action.js (Diff revision 3)
     
     
     
     

    The first parameter can probably just use |json_dumps without the other {}, since it's not merging it in with anything.

  22. 
      
david
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

david
Review request changed

Change Summary:

Check parent should_render so menus can entirely control their children.

Commits:

Summary
-
Add base classes and registry for new actions framework.
+
Add base classes and registry for new actions framework.

Diff:

Revision 5 (+2328)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

david
Review request changed

Change Summary:

  • Change element classes to be generic names (no "review-request")
  • Add icon_class attribute.

Commits:

Summary
-
Add base classes and registry for new actions framework.
+
Add base classes and registry for new actions framework.

Diff:

Revision 6 (+2358)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

maubin
  1. 
      
  2. reviewboard/actions/__init__.py (Diff revision 6)
     
     

    Missing a period.

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

    I think it'd be good to add some docstrings to these explaining what each attachment point is, like what it corresponds to in the UI.

  4. reviewboard/actions/templatetags/actions.py (Diff revision 6)
     
     
     

    Should we include the exception details in the log?

    1. logger.exception does that itself.

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

    Should we include the exception details in the log?

  6. Could add -> None to the rest of these test methods.

  7. 
      
david
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

david
Review request changed

Change Summary:

  • Port JS to spina
  • Move registry initialization with built-in actions into the change that adds those actions.

Commits:

Summary
-
Add base classes and registry for new actions framework.
+
Add base classes and registry for new actions framework.

Diff:

Revision 8 (+2340)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

david
Review request changed

Change Summary:

Make menu action's menu public so subclasses can use it directly.

Commits:

Summary
-
Add base classes and registry for new actions framework.
+
Add base classes and registry for new actions framework.

Diff:

Revision 9 (+2340)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

chipx86
  1. This looks great!

    Pretty much everything here is tiny, with some being "what standards do we want to set for TypeScript code?" comments.

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

    Since these are documented as instance variables, we no longer need to have these here.

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

    Pure nit, but maybe alphabetize these, so it's clear where new entries should go?

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

    For most of these functions, we can err on the side of making things like context and maybe request keyword-only arguments? I think that's a good habit we should get into for things meant to be overridden by subclasses, so we're not stuck maintaining argument orders indefinitely.

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

    Can we order these alphabetically?

  6. reviewboard/actions/errors.py (Diff revision 9)
     
     
     
     

    Can we make at least depth_limit a keyword-only argument?

  7. reviewboard/actions/errors.py (Diff revision 9)
     
     

    Do we want to localize this string?

    1. I don't think that's necessary. This error will only apply to us and extension authors, it's not user-visible.

  8. reviewboard/actions/registry.py (Diff revision 9)
     
     
     
     
     
     
     
     
     
     

    Descriptions are a little inconsistent. We can probably omit the "Raised" parts.

  9. reviewboard/actions/registry.py (Diff revision 9)
     
     

    Can we add an explicit typing for parent at the top of the function? I've seen either mypy or pyright sometimes get things wrong without it (or convert to Any, depending) when there's some assignments to values, some to None.

  10. reviewboard/actions/registry.py (Diff revision 9)
     
     
     
     
     

    Mind adding a comment with what you were telling me during my previous review, so it's clear that there may be multiple things being unregistered here?

  11. reviewboard/actions/registry.py (Diff revision 9)
     
     
     

    This can use yield from.

  12. We may need to type this.

  13. We may need to type this.

  14. We may need to type this.

  15. We should probably document our interfaces and their types.

    I don't know what the right thing is for then documenting the model's attributes. Maybe we just rely on the interface's going forward.

  16. I almost left an entirely different comment, but now I realize Action is meant to be a generic. Might be worth documenting this and that subclasses may want to provide a subclass for their attributes?

  17. I've kind of been adopting Python's import groups rules. I think that's a nice way of keeping things a bit separate and clear. Can we do that for these changes?

    1. To indicate it as a member. I just forgot to add the type there.

  18. Every time .render() is called, we'll get a new menu appended. I'm assuming we don't want that? Should this be onInitialRender(), or should we clear elements and clean up state?

  19. When RB.MenuView is converted, we'll be able to do:

    this.menu.renderInto(this.el);
    
  20. Since .children() wraps things, we should probably just access menu.el.children.length directly.

  21. Do we want these to be private?

    1. I had initially done so but at least for onKeyDown I need to be able to have a subclass override it and call super(). I can add protected.

  22. A switch might be a better choice here for all these. I think JavaScript can optimize that better too.

    1. I always forget that JS can switch on strings.

    2. It turns out that switch statements are only faster when dealing with number types, and can be slower than if statements for strings. Given that, and that eslint throws a fit with this sort of switch statement, I think I'm going to change this back.

  23. 
      
david
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

chipx86
  1. 
      
  2. Can we have a doc comment for the interface?

  3. Can we have a doc comment for the interface?

  4. Alphabetical order? Just to keep it manageable.

    1. As I commented above, I'm going to change this back to an if.

  5. 
      
david
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

david
Review request changed

Change Summary:

Fix arguments passed to MenuView.open

Commits:

Summary
-
Add base classes and registry for new actions framework.
+
Add base classes and registry for new actions framework.

Diff:

Revision 12 (+2426)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

david
Review request changed

Change Summary:

Pull a couple things from later changes into this one.

Commits:

Summary
-
Add base classes and registry for new actions framework.
+
Add base classes and registry for new actions framework.

Diff:

Revision 13 (+2532 -4)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

chipx86
  1. 
      
  2. reviewboard/static/rb/js/ui/views/menuView.es6.js (Diff revision 13)
     
     
     
     
     

    Can we add a Version Added here, and a Version Changed on the function description to document it?

  3. 
      
david
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

chipx86
  1. 
      
  2. Let's go with import groups and do a blank line between these.

  3. 
      
david
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-6.x (65e08ad)
Loading...