Switch scm client discovery to entry points

Review Request #4145 — Created May 13, 2013 and submitted

smacleod
RBTools
master
rbtools
chipx86
Switch scm client discovery to entry points.

SCM Clients will now be discovered using entry points, instead of a
hardcoded list. This will allow third party packages to provide
support for additional clients.

Additionally, a '--repository-type' option has been added to all of
the rbt commands and post-review. This option allows the user to
specify a particular repository type, which the working directory will
be checked for before other repository types.

The valid repository types can be printed using the new
'--list-repository-types' option to post-review, or by invoking the
new 'rbt list-repo-types' command.
Used the --repository-type option and confirmed the provided type was
being checked initially (By observing the debug messages).
Description From Last Updated

Blank line.

chipx86chipx86

We should catch any exceptions and log.

chipx86chipx86

We duplicate so many of these. Maybe we should have a file where we define common options. Either way, above ...

chipx86chipx86

Same here as above. This is a bit long. Maybe leave off the valid types, and instead add a --list-repository-types ...

chipx86chipx86

I think this can be compacted a bit to prevent multiple places looking for repo info. How about: if client_name: ...

chipx86chipx86

Would prefer this be spread across 3 lines, like: scmclients = { client_name: ..., }

chipx86chipx86

Can do elif/else here.

chipx86chipx86

I'd like to use single quotes instead of double where possible. It's cleaner-looking.

chipx86chipx86

Indentation error.

chipx86chipx86
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        rbtools/commands/status.py
        rbtools/postreview.py
        rbtools/commands/post.py
        rbtools/commands/__init__.py
        rbtools/commands/diff.py
        rbtools/commands/patch.py
        rbtools/clients/__init__.py
        rbtools/commands/attach.py
        rbtools/commands/publish.py
        setup.py
        rbtools/commands/close.py
      Ignored Files:
        docs/rbtools/rbt/commands/status.txt
        docs/rbtools/rbt/commands/patch.txt
        docs/rbtools/rbt/commands/diff.txt
        docs/rbtools/rbt/commands/close.txt
        docs/rbtools/rbt/commands/post.txt
        docs/rbtools/rbt/commands/publish.txt
        docs/rbtools/rbt/commands/attach.txt
        docs/rbtools/post-review.txt
    
    
  2. 
      
chipx86
  1. 
      
  2. docs/rbtools/post-review.txt (Diff revision 1)
     
     
    Comma before "but".
  3. docs/rbtools/post-review.txt (Diff revision 1)
     
     
     
    I'd put a colon after "include", and maybe ``..`` around each type.
    
    This and the previous apply to all other docs.
  4. rbtools/postreview.py (Diff revision 1)
     
     
     
     
     
     
    Same here as above.
    
    This is a bit long. Maybe leave off the valid types, and instead add a --list-repository-types option for listing them.
    1. I'm going to add that option for post-review, but instead of adding that logic to every rbt command that takes --repository-type, I'm just going to add a simple 'rbt list-repo-types'.
  5. 
      
chipx86
  1. 
      
  2. rbtools/clients/__init__.py (Diff revision 1)
     
     
     
    Blank line.
  3. rbtools/clients/__init__.py (Diff revision 1)
     
     
    We should catch any exceptions and log.
  4. rbtools/clients/__init__.py (Diff revision 1)
     
     
     
     
     
     
     
    My feeling is that if they specify a type, and it doesn't match, we shouldn't try anything else. Otherwise, we might hit some parent directory's matched repository, or the global $P4PORT-based repo info for Perforce.
    1. Sounds good. I couldn't decide if having the fall back autodetection was a good idea
      or not, so I just went with it arbitrarily.
  5. rbtools/commands/attach.py (Diff revision 1)
     
     
     
     
     
     
     
     
    We duplicate so many of these. Maybe we should have a file where we define common options.
    
    Either way, above comments apply to this and other help strings.
    1. Yeah, that's on the roadmap. I'm going to consolidate the options into
      different functional groups (e.g. Server options, SCM Options, etc.)
      and allow the commands to grab the common options they need (so if they
      talk to the SCM, they need the SCM options). I'm going to leave that out
      of this change though.
  6. 
      
SM
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        rbtools/commands/status.py
        rbtools/postreview.py
        rbtools/commands/post.py
        rbtools/commands/__init__.py
        rbtools/commands/diff.py
        rbtools/commands/patch.py
        rbtools/clients/__init__.py
        rbtools/commands/attach.py
        rbtools/commands/publish.py
        setup.py
        rbtools/commands/close.py
        rbtools/commands/list_repo_types.py
      Ignored Files:
        docs/rbtools/rbt/commands/attach.txt
        docs/rbtools/rbt/commands/status.txt
        docs/rbtools/rbt/commands/patch.txt
        docs/rbtools/rbt/commands/diff.txt
        docs/rbtools/rbt/commands/close.txt
        docs/rbtools/rbt/commands/list-repo-types.txt
        docs/rbtools/rbt/commands/post.txt
        docs/rbtools/rbt/commands/publish.txt
        docs/rbtools/rbt/commands/index.txt
        docs/rbtools/post-review.txt
    
    
  2. 
      
chipx86
  1. 
      
  2. rbtools/clients/__init__.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    I think this can be compacted a bit to prevent multiple places looking for repo info. How about:
    
    if client_name:
        if client_name not in SCMCLIENTS:
            logging.error('The provided ... is invalid')
            sys.exit(1)
        else:
            scmclients = [client_name]
    else:
        scmclients = SCMCLIENTS
    
    for name, tool in scmclients.iteritems():
        ...
    
    if not repository.info:
        if client_name:
            logging.error('The provided repository type ...')
    
        ...
    1. Much nicer, thanks
  3. rbtools/commands/list_repo_types.py (Diff revision 2)
     
     
     
     
    I'd like to use single quotes instead of double where possible. It's cleaner-looking.
  4. rbtools/commands/list_repo_types.py (Diff revision 2)
     
     
    Indentation error.
  5. 
      
SM
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        rbtools/commands/status.py
        rbtools/postreview.py
        rbtools/commands/post.py
        rbtools/commands/__init__.py
        rbtools/commands/diff.py
        rbtools/commands/patch.py
        rbtools/clients/__init__.py
        rbtools/commands/attach.py
        rbtools/commands/publish.py
        setup.py
        rbtools/commands/close.py
        rbtools/commands/list_repo_types.py
      Ignored Files:
        docs/rbtools/rbt/commands/attach.txt
        docs/rbtools/rbt/commands/status.txt
        docs/rbtools/rbt/commands/patch.txt
        docs/rbtools/rbt/commands/diff.txt
        docs/rbtools/rbt/commands/close.txt
        docs/rbtools/rbt/commands/list-repo-types.txt
        docs/rbtools/rbt/commands/post.txt
        docs/rbtools/rbt/commands/publish.txt
        docs/rbtools/rbt/commands/index.txt
        docs/rbtools/post-review.txt
    
    
  2. 
      
chipx86
  1. Only two small things, then ship it!
  2. rbtools/clients/__init__.py (Diff revisions 2 - 3)
     
     
    Would prefer this be spread across 3 lines, like:
    
    scmclients = {
       client_name: ...,
    }
  3. rbtools/clients/__init__.py (Diff revisions 2 - 3)
     
     
     
     
     
     
     
     
    Can do elif/else here.
  4. 
      
SM
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master, release-0.5.x (4524ef3c1f09c44f7bd8dab78ed171f42adebd72)
Loading...