I think this would be cleaner if we build repository_type_list in one loop (or even better, a list comprehension), and then we can either print it out directly as json or loop through it again to print in plain-text:

repository_type_list = [
    {
        'name': name,
        'tool_name': tool.name,
        'detected': bool(tool.get_repository_info()),
    }
    for name, tool in six.iteritems(SCMCLIENTS)
]

if options.json:
    print_json({
        'repository_types': repository_type_list,
    })
else:
    for repository_type in repository_type_list:
        if repository_type['detected']:
            dectected = '*'
        else:
            dectected = ' '

        print(' %s "%s": %s' % (detected,
                                repository_type['name'],
                                repository_type['tool_name']))

We should use six.text_type() instead of str() for future Python 3 compatibility. Here and lots of other places throughout your change.

Your output in this command is a little inconsistent. Here you have file_path as the key but below you use path_to_file.

I suspect we want to change these uses of logging.info() to be print() like the other commands.

I suspect we want to change these uses of logging.info() to be print() like the other commands.

How does this change the output?

By printing here, it looks like we'll end up printing multiple JSON objects out? That doesn't seem correct. Instead of splatting in a print_json() call anywhere there might be a print() statement we should probably collect the information about what happened and then only print at the very end.

Can you put the { on the next line and then indent things within the dict another 4 spaces?

} should be on a separate line from the for .... We should also add a trailing comma after the ]

It would be nice to pass in the indent parameter to json.dumps so that we get easier-to-read output.

This is a utility method called from within other commands. If this ends up printing output it seems like we might end up with multiple JSON objects printed, which won't parse correctly. We should only be calling print_json from things inside rbtools/commands/*.py

I think we probably want to refactor this so that the code in rbtools/clients/ returns the data, and then rbtools/commands/list_repo_types.py can actually do the printing. Anything that's "library" code shouldn't print JSON, since we might end up printing more than one JSON object.

These changes seem unrelated. Why did you do this?

This should have an "Args" section explaining what obj is. The summary line also needs to end in a period.

This should use the standard "Returns:" section, plus it doesn't return a bool, it returns a dict with some fields.
This also needs an "Args" section explaining what cache_path is.

Instead of defining this as a variable and then manipulating it throughout, because we only have two cases, how about something like this:

try:
    os.unlink(cache_path)
    return {
        'success': True,
        'cache_path': cache_path,
    }
except Exception as e:
    logging.error(...)
    return {
        'success': False,
        'error_message': 'Could not clear cache',
        'exception_message': six.text_type(e),
    }

Perhaps we want to rename this method now?

Should use the standard Args/Returns sections.

Kind of silly to define a variable and immediately return it. Lets just do return [ ... ]

This can be a bit less wordy/more efficient as:

json_output_dict.extend({
    'squash': self.options.squash,
    'merge': not self.options.squash,
    'source_branch': branch_name,
    'destination_branch': destination_branch,
})

Same here re: dict.extend

While we don't localize rbtools right now, it's possible we will in the future, and this won't be translatable. We should define separate format strings in the conditional, even though they're substantially similar:

if was_authenticated:
    api_client.logout()
    message_fmt = 'You are now logged out of Review Board at %s'
else:
    message_fmt = 'You are already logged out of Review Board at %s'

message = message_fmt % (api_client.domain)

Same here with msg_input not being localizable.

Anywhere we're using msg_input here should have a conditional with separate strings.

We generally prefer parens for wrapping this sort of thing instead of the line continuation character. We probably also want the format parameters on a separate line:

error_msg = ('No %s repository found or selected for %s. %s not '
             'created.'
             % (tool.name, server, CONFIG_FILE))

su before sy.

Just a few more nits to pick.

We generally document these like this:

Returns:
    dict:
    A dictionary with the following keys:

    ``success``:
        A boolean value for whether the cache was cleared.

    ``cache_path``:
        The path of the cache file that was cleared.

    In the case of failure, this will also include two additional keys:

    ``error_message``:
        A user-visible error message.

    ``exception_message``:
        The message from the exception that caused the error.

Should probably be "List of dicts, each containing name, tool_name, and detected fields."

Other commands include a success field. We probably want that here even though it will always be True.

Can use json_output_dict.extend to do this all in one go.

Can use json_output_dict.extend to do this all in one go.

And here.

And finally here.

Type should be unicode, not string (string isn't a python type)

We need to list the types of config and options (dict and object, respectively). There should also be a blank line between them.

Does raise CommandError(err) output to stdout or to stderr? Also, it looks like in the case where the --json option is set, that 2 error messages are generated. Is this the case? If so, is it intentional?

Same question here.

And here. And many more below.