Summary

Switch scm client discovery to entry points

Review Request #4145 — Created May 13, 2013 and submitted May 29, 2013, 10:23 p.m.

Information

Owner

smacleod

Repository

RBTools

Branch

master

Bugs

Depends On

Reviewers

Groups

rbtools

People

chipx86

Description

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.

Testing Done

Used the --repository-type option and confirmed the provided type was
being checked initially (By observing the debug messages).

Issues

Description	From	Last Updated
Blank line.	chipx86	May 28, 2013, 11:47 a.m.
We should catch any exceptions and log.	chipx86	May 28, 2013, 11:49 a.m.
We duplicate so many of these. Maybe we should have a file where we define common options. Either way, above …	chipx86	May 28, 2013, 11:57 a.m.
Same here as above. This is a bit long. Maybe leave off the valid types, and instead add a --list-repository-types …	chipx86	May 28, 2013, 11:26 a.m.
I think this can be compacted a bit to prevent multiple places looking for repo info. How about: if client_name: …	chipx86	May 29, 2013, 11:08 a.m.
Would prefer this be spread across 3 lines, like: scmclients = { client_name: ..., }	chipx86	May 29, 2013, 7:04 p.m.
Can do elif/else here.	chipx86	May 29, 2013, 7:06 p.m.
I'd like to use single quotes instead of double where possible. It's cleaner-looking.	chipx86	May 29, 2013, 11:09 a.m.
Indentation error.	chipx86	May 29, 2013, 11:09 a.m.

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

docs/rbtools/post-review.txt (Diff revision 1)
```
Comma before "but".
```

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.

rbtools/postreview.py (Diff revision 1)

The issue has been resolved. Show all issues

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.

smacleod May 28, 2013, 1:29 p.m.

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'.

rbtools/clients/__init__.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Blank line.
```
rbtools/clients/__init__.py (Diff revision 1)
The issue has been resolved. Show all issues
```
We should catch any exceptions and log.
```

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.

smacleod May 28, 2013, 1:29 p.m.

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.

rbtools/commands/attach.py (Diff revision 1)

The issue has been dropped. Show all issues

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.

smacleod May 28, 2013, 1:29 p.m.

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.

Change Summary:

Updated based on Christian's review. Introduced a new 'rbt list-repo-types' command.

Description:

		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.

Branch:

master

release-0.5.x

People:

chipx86

Diff:

Revision 2 (+339 -42)

Show changes

	setup.py
	docs/rbtools/post-review.txt
	docs/rbtools/rbt/commands/attach.txt
	docs/rbtools/rbt/commands/close.txt
	docs/rbtools/rbt/commands/diff.txt
	docs/rbtools/rbt/commands/index.txt
	docs/rbtools/rbt/commands/list-repo-types.txt
	docs/rbtools/rbt/commands/patch.txt
	14 more

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

rbtools/clients/__init__.py (Diff revision 2)

The issue has been resolved. Show all issues

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 ...')

    ...

SM

smacleod May 29, 2013, 11:09 a.m.
```
Much nicer, thanks
```

rbtools/commands/list_repo_types.py (Diff revision 2)

The issue has been resolved. Show all issues

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

rbtools/commands/list_repo_types.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Indentation error.
```

Change Summary:

Updated based on Christian's review.

Branch:

release-0.5.x

master

Diff:

Revision 3 (+338 -42)

Show changes

	setup.py
	docs/rbtools/post-review.txt
	docs/rbtools/rbt/commands/attach.txt
	docs/rbtools/rbt/commands/close.txt
	docs/rbtools/rbt/commands/diff.txt
	docs/rbtools/rbt/commands/index.txt
	docs/rbtools/rbt/commands/list-repo-types.txt
	docs/rbtools/rbt/commands/patch.txt
	14 more

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

Ship it!

```
Only two small things, then ship it!
```

rbtools/clients/__init__.py (Diff revisions 2 - 3)

The issue has been resolved. Show all issues

Would prefer this be spread across 3 lines, like:

scmclients = {
   client_name: ...,
}

rbtools/clients/__init__.py (Diff revisions 2 - 3)

The issue has been resolved. Show all issues

Can do elif/else here.

Status:: Completed
Change Summary:: Pushed to master, release-0.5.x (4524ef3c1f09c44f7bd8dab78ed171f42adebd72)