Switch scm client discovery to entry points
Review Request #4145 — Created May 13, 2013 and submitted
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. |
 chipx86
|
|
|
We should catch any exceptions and log. |
chipx86
|
|
|
We duplicate so many of these. Maybe we should have a file where we define common options. Either way, above … |
chipx86
|
|
|
Same here as above. This is a bit long. Maybe leave off the valid types, and instead add a --list-repository-types … |
chipx86
|
|
|
I think this can be compacted a bit to prevent multiple places looking for repo info. How about: if client_name: … |
chipx86
|
|
|
Would prefer this be spread across 3 lines, like: scmclients = { client_name: ..., } |
chipx86
|
|
|
Can do elif/else here. |
chipx86
|
|
|
I'd like to use single quotes instead of double where possible. It's cleaner-looking. |
chipx86
|
|
|
Indentation error. |
chipx86
|
-
-
-
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) 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.
-
-
-
-
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.
-
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.
SM
Change Summary:
Updated based on Christian's review. Introduced a new 'rbt list-repo-types' command.
Description: |
|
||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Branch: |
|
||||||||||||||||||||||||||||||||||||||||||
People: |
|
||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 2 (+339 -42) |
-
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) 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 ...') ... -
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.
-
SM
Change Summary:
Updated based on Christian's review.
Branch: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+338 -42) |
-
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
-
Only two small things, then ship it!
-
rbtools/clients/__init__.py (Diff revisions 2 - 3) Would prefer this be spread across 3 lines, like: scmclients = { client_name: ..., } -
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