Make RBTools command initialization easier and more consistent.

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

david
RBTools
master
rbtools

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
Make RBTools command initialization easier and more consistent.
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
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. 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)
     
     
     

    Typo: "repat"

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

    Private methods should go below public.

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

    "command line"

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

    Should we just assign server_url unconditionally?

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

    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)
     
     

    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)
     
     

    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)
     
     

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

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

    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)
     
     
     
     
     
     
     

    Missing a Raises

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

    While here, can we make these keyword arguments?

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

    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)
     
     
     
     
     
     
     
     

    Missing a Raises:.

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

    Let's pull out tool as well.

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

    Let's pull out repository_info.

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

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

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

    Can we use keyword arguments here?

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

    And here.

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

    Can we use keyword arguments here?

  22. 
      
david
Review request changed

Commits:

Summary
-
Make RBTools command initialization easier and more consistent.
+
Make RBTools command initialization easier and more consistent.

Branch:

-release-2.0.x
+master

Diff:

Revision 3 (+970 -766)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...