• 
      

    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)