Make RBTools command initialization easier and more consistent.

Review Request #11480 — Created Feb. 22, 2021 and submitted

Information

RBTools
master

Reviewers

The different commands in RBTools have lots of differerent needs. Some
of them interact only with your local repository, some of them interact
only with the server API, some do both, and some do neither. We've
traditionally had a few methods to handle things like setting up the API
or the SCMClient, but usage of these was inconsistent and resulted in a
lot of repeated boilerplate. Because of this and the history of RBTools
development, usage of all of this was somewhat inconsistent, with many
commands doing more or less than they really needed to do.

This change is the first of a couple to clean this up. Two new
attributes have been added to the base Command class: needs_api and
needs_scm_client. Subclasses can set these to True depending on
their needs, and then prior to main being called, the initialization
process will set various attributes: api_client, api_root,
capabilities, repository_info, server_url, and tool.

This change handles most of the initialization process. The one major
remaining piece are the calls to find_server_repository_info, which
will require some more involved changes (for example, short-cutting in
the case when REPOSITORY_NAME or REPOSITORY_URL are defined).

  • Smoke tested basic functionality for all changed commands.
  • Verified rbt patch behavior was correct with both ID and full URL
    (ID only and ID + diff revision) arguments. Checked that
    rbt patch --print did not initialize any repository support.
  • Verified rbt land behavior was correct with both ID and full URL
    --review-request-id option.
  • Checked that rbt status --all did not initialize any repository
    support.
Summary ID
Make RBTools command initialization easier and more consistent.
The different commands in RBTools have lots of differerent needs. Some of them interact only with your local repository, some of them interact only with the server API, some do both, and some do neither. We've traditionally had a few methods to handle things like setting up the API or the SCMClient, but usage of these was inconsistent and resulted in a lot of repeated boilerplate. Because of this and the history of RBTools development, usage of all of this was somewhat inconsistent, with many commands doing more or less than they really needed to do. This change is the first of a couple to clean this up. Two new attributes have been added to the base `Command` class: `needs_api` and `needs_scm_client`. Subclasses can set these to `True` depending on their needs, and then prior to `main` being called, the initialization process will set various attributes: `api_client`, `api_root`, `capabilities`, `repository_info`, `server_url`, and `tool`. This change handles most of the initialization process. The one major remaining piece are the calls to `find_server_repository_info`, which will require some more involved changes (for example, short-cutting in the case when REPOSITORY_NAME or REPOSITORY_URL are defined). Testing Done: - Smoke tested basic functionality for all changed commands. - Verified `rbt patch` behavior was correct with both ID and full URL (ID only and ID + diff revision) arguments. Checked that `rbt patch --print` did not initialize any repository support. - Verified `rbt land` behavior was correct with both ID and full URL `--review-request-id` option. - Checked that `rbt status --all` did not initialize any repository support.
7a393157dfa946d52898718a1e5d0a969df39591
Description From Last Updated

Let's move this work to master for 3.0, given how much of a change I suspect this will all ultimately …

chipx86chipx86

F821 undefined name 'tool'

reviewbotreviewbot

F821 undefined name 'repository_info'

reviewbotreviewbot

F821 undefined name 'repository_info'

reviewbotreviewbot

F821 undefined name 'repository_info'

reviewbotreviewbot

F821 undefined name 'repository_info'

reviewbotreviewbot

F821 undefined name 'repository_info'

reviewbotreviewbot

Typo: "repat"

chipx86chipx86

Private methods should go below public.

chipx86chipx86

"command line"

chipx86chipx86

Should we just assign server_url unconditionally?

chipx86chipx86

This should be one if statement. It's checking server_url is None twice, and there's never an alternate condition for the …

chipx86chipx86

This doesn't handle the case of trees[path] not existing.

chipx86chipx86

This should be logging.warning. The warn alias is, as far as I can tell, for backwards-compatibility only. Actually though, I …

chipx86chipx86

This should also be updated to use warnings and ideally a version-specific DeprecationWarning.

chipx86chipx86

Can we pull out self.tool into a local variable to avoid the repeated attribute lookups and line changes?

chipx86chipx86

Missing a Raises

chipx86chipx86

While here, can we make these keyword arguments?

chipx86chipx86

Let's pull out api_client to avoid repeated attribute lookups.

chipx86chipx86

Missing a Raises:.

chipx86chipx86

Let's pull out tool as well.

chipx86chipx86

Let's pull out repository_info.

chipx86chipx86

While here, can we make these all keyword arguments? This is a really wordy function.

chipx86chipx86

Can we use keyword arguments here?

chipx86chipx86

And here.

chipx86chipx86

Can we use keyword arguments here?

chipx86chipx86

How about we call this initialize? That'll be consistent with the naming used for extensions.

chipx86chipx86

Does this fit on one line now?

chipx86chipx86

Can you add a Version Added to these (before Type)?

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

flake8

david
chipx86
  1. This is a great cleanup! This cleanly removes one of the biggest annoyances with writing commands. I love it.

    My biggest nit is that I'd like to move toward fewer, rather than more, attribute accesses. We have a lot of code (existing and newly-introduced in this change) that does a lot of repeated self.some_attr_name, and while it's not performance-critical code, it's slower and less readable to keep doing that. I suspect that this change will shrink a bit if we replace some of the older code you're getting rid of with statements like tool = self.tool when that variable's going to be used repeatedly.

    I marked a few cases like this, but it's not comprehensive.

    There's a few other areas of code that I've always disliked, where we're calling some utility function with a bunch of positional arguments, and I think there's a good opportunity to switch those calls to keyword arguments. I only marked up a few of the bigger cases here.

  2. Show all issues

    Let's move this work to master for 3.0, given how much of a change I suspect this will all ultimately be.

  3. rbtools/commands/__init__.py (Diff revision 2)
     
     
     
    Show all issues

    Typo: "repat"

  4. rbtools/commands/__init__.py (Diff revision 2)
     
     
    Show all issues

    Private methods should go below public.

  5. rbtools/commands/__init__.py (Diff revision 2)
     
     
    Show all issues

    "command line"

  6. rbtools/commands/__init__.py (Diff revision 2)
     
     
     
    Show all issues

    Should we just assign server_url unconditionally?

  7. rbtools/commands/__init__.py (Diff revision 2)
     
     
     
    Show all issues

    This should be one if statement. It's checking server_url is None twice, and there's never an alternate condition for the outer if.

  8. rbtools/commands/__init__.py (Diff revision 2)
     
     
    Show all issues

    This doesn't handle the case of trees[path] not existing.

    1. That's handled in the structure of the for loop above.

  9. rbtools/commands/__init__.py (Diff revision 2)
     
     
    Show all issues

    This should be logging.warning. The warn alias is, as far as I can tell, for backwards-compatibility only.

    Actually though, I think we want to use the warnings module. Ideally, and this can be a follow-up change if you want, we should have a rbtools.deprecation module that provides classes similar to Review Board's, so we can put together a proper deprecation strategy.

    1. I'll switch it to logging.warning but the deprecation module stuff seems better suited for a separate change.

  10. rbtools/commands/__init__.py (Diff revision 2)
     
     
    Show all issues

    This should also be updated to use warnings and ideally a version-specific DeprecationWarning.

  11. rbtools/commands/diff.py (Diff revision 2)
     
     
    Show all issues

    Can we pull out self.tool into a local variable to avoid the repeated attribute lookups and line changes?

  12. rbtools/commands/land.py (Diff revision 2)
     
     
     
     
     
     
     
    Show all issues

    Missing a Raises

  13. rbtools/commands/login.py (Diff revision 2)
     
     
     
    Show all issues

    While here, can we make these keyword arguments?

  14. rbtools/commands/logout.py (Diff revision 2)
     
     
    Show all issues

    Let's pull out api_client to avoid repeated attribute lookups.

    1. Saving one extra attribute lookup in a command which is almost never used doesn't seem worth the extra line.

  15. rbtools/commands/patch.py (Diff revision 2)
     
     
     
     
     
     
     
     
    Show all issues

    Missing a Raises:.

  16. rbtools/commands/patch.py (Diff revision 2)
     
     
    Show all issues

    Let's pull out tool as well.

  17. rbtools/commands/post.py (Diff revision 2)
     
     
    Show all issues

    Let's pull out repository_info.

  18. rbtools/commands/post.py (Diff revision 2)
     
     
     
     
     
     
     
    Show all issues

    While here, can we make these all keyword arguments? This is a really wordy function.

  19. rbtools/commands/stamp.py (Diff revision 2)
     
     
     
     
    Show all issues

    Can we use keyword arguments here?

  20. rbtools/commands/stamp.py (Diff revision 2)
     
     
     
     
     
     
     
    Show all issues

    And here.

  21. rbtools/commands/status.py (Diff revision 2)
     
     
     
    Show all issues

    Can we use keyword arguments here?

  22. 
      
david
chipx86
  1. Looks good. I just have one suggestion regarding naming.

  2. rbtools/commands/__init__.py (Diff revision 3)
     
     
    Show all issues

    How about we call this initialize? That'll be consistent with the naming used for extensions.

  3. rbtools/commands/post.py (Diff revision 3)
     
     
     
     
    Show all issues

    Does this fit on one line now?

  4. 
      
david
david
chipx86
  1. 
      
  2. rbtools/commands/__init__.py (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Can you add a Version Added to these (before Type)?

  3. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to master (35275e6)