Add --json flag to format rbt output as json.

Review Request #9245 — Created Oct. 3, 2017 and discarded

Information

RBTools
release-0.7.x

Reviewers

When running the rbt command from a script, it is difficult to parse
the output log messages in order to get necessary information from the
command that was run.

This change adds a --json flag to all rbt commands to change the
output to json to make command output parsing easier from a script.

The rbt diff command hasn't been changed because diff output is
expected on stdout.

All rbt tests passed.

Description From Last Updated

I think this would be cleaner if we build repository_type_list in one loop (or even better, a list comprehension), and …

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

How does this change the output?

daviddavid

By printing here, it looks like we'll end up printing multiple JSON objects out? That doesn't seem correct. Instead of …

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

This is a utility method called from within other commands. If this ends up printing output it seems like we …

daviddavid

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 …

daviddavid

These changes seem unrelated. Why did you do this?

daviddavid

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

daviddavid

This should use the standard "Returns:" section, plus it doesn't return a bool, it returns a dict with some fields. …

daviddavid

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

daviddavid

Perhaps we want to rename this method now?

daviddavid

Should use the standard Args/Returns sections.

daviddavid

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

daviddavid

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, })

daviddavid

Same here re: dict.extend

daviddavid

While we don't localize rbtools right now, it's possible we will in the future, and this won't be translatable. We …

daviddavid

Same here with msg_input not being localizable.

daviddavid

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

daviddavid

We generally prefer parens for wrapping this sort of thing instead of the line continuation character. We probably also want …

daviddavid

su before sy.

daviddavid

We generally document these like this: Returns: dict: A dictionary with the following keys: ``success``: A boolean value for whether …

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

And here.

daviddavid

And finally here.

daviddavid

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

daviddavid

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

daviddavid
bleblan2
bleblan2
bleblan2
david
  1. 
      
  2. rbtools/clients/__init__.py (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    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']))
    
  3. rbtools/commands/api_get.py (Diff revision 4)
     
     
    Show all issues

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

  4. rbtools/commands/attach.py (Diff revision 4)
     
     
    Show all issues

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

  5. rbtools/commands/login.py (Diff revision 4)
     
     
    Show all issues

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

  6. rbtools/commands/logout.py (Diff revision 4)
     
     
    Show all issues

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

  7. rbtools/commands/patch.py (Diff revision 4)
     
     
    Show all issues

    How does this change the output?

    1. This will change the output of the warning to be printed to stderr instead of stdout. This lets us log the warning without it interfering with the json formatted output on stdout if the json option is specified. If the warning is expected to be on stdout I can change this line to:

      if self.options.json:
          logging.warning(message)
      else:
          print(message)
      
  8. rbtools/commands/post.py (Diff revision 4)
     
     
     
     
    Show all issues

    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.

  9. rbtools/commands/status.py (Diff revision 4)
     
     
    Show all issues

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

  10. rbtools/commands/status.py (Diff revision 4)
     
     
     
    Show all issues

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

  11. rbtools/utils/console.py (Diff revision 4)
     
     
    Show all issues

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

  12. 
      
bleblan2
bleblan2
bleblan2
bleblan2
bleblan2
david
  1. 
      
  2. rbtools/api/cache.py (Diff revision 8)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    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

  3. rbtools/clients/__init__.py (Diff revision 8)
     
     
    Show all issues

    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.

  4. rbtools/utils/console.py (Diff revision 8)
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    These changes seem unrelated. Why did you do this?

    1. This is because the string passed to input() is output on stdout. If an rbt command is invoked with --json and the command has interactive questions to ask, they would've been output on stdout as well as the single json object which would break any attempt at parsing json from the stdout. With the output of the question on stderr, we can still output only the single json object to stdout even if there is interactive questions asked to the user which won't break json parsing.

  5. rbtools/utils/console.py (Diff revision 8)
     
     
    Show all issues

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

  6. 
      
bleblan2
david
  1. 
      
  2. rbtools/api/cache.py (Diff revision 9)
     
     
     
    Show all issues

    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.

  3. rbtools/api/cache.py (Diff revision 9)
     
     
     
     
     
    Show all issues

    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),
        }
    
  4. rbtools/clients/__init__.py (Diff revision 9)
     
     
    Show all issues

    Perhaps we want to rename this method now?

  5. rbtools/clients/__init__.py (Diff revision 9)
     
     
     
    Show all issues

    Should use the standard Args/Returns sections.

  6. rbtools/clients/__init__.py (Diff revision 9)
     
     
    Show all issues

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

  7. rbtools/commands/land.py (Diff revision 9)
     
     
     
     
     
    Show all issues

    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,
    })
    
  8. rbtools/commands/land.py (Diff revision 9)
     
     
     
    Show all issues

    Same here re: dict.extend

  9. rbtools/commands/logout.py (Diff revision 9)
     
     
     
     
     
     
     
    Show all issues

    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)
    
  10. rbtools/commands/patch.py (Diff revision 9)
     
     
     
    Show all issues

    Same here with msg_input not being localizable.

  11. rbtools/commands/patch.py (Diff revision 9)
     
     
    Show all issues

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

  12. rbtools/commands/setup_repo.py (Diff revision 9)
     
     
     
    Show all issues

    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))
    
  13. rbtools/utils/console.py (Diff revision 9)
     
     
     
    Show all issues

    su before sy.

  14. 
      
bleblan2
bleblan2
david
  1. Just a few more nits to pick.

  2. rbtools/api/cache.py (Diff revision 11)
     
     
     
     
     
     
    Show all issues

    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.
    
  3. rbtools/clients/__init__.py (Diff revision 11)
     
     
     
    Show all issues

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

  4. rbtools/commands/list_repo_types.py (Diff revision 11)
     
     
    Show all issues

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

  5. rbtools/commands/patch.py (Diff revision 11)
     
     
     
    Show all issues

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

  6. rbtools/commands/patch.py (Diff revision 11)
     
     
     
     
     
    Show all issues

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

  7. rbtools/commands/patch.py (Diff revision 11)
     
     
     
    Show all issues

    And here.

  8. rbtools/commands/patch.py (Diff revision 11)
     
     
     
    Show all issues

    And finally here.

  9. 
      
bleblan2
david
  1. 
      
  2. rbtools/api/cache.py (Diff revision 12)
     
     
    Show all issues

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

  3. rbtools/clients/__init__.py (Diff revision 12)
     
     
     
     
     
    Show all issues

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

  4. 
      
bleblan2
hmarceau
  1. 
      
  2. rbtools/commands/install.py (Diff revision 13)
     
     

    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?

    1. So raise CommandError(err) outputs to stderr. The goal of the --json flag is to have the stdout from the command be specifically a single json formatted object so that stdout can be piped into another process that parses that json. This means that anything not outputted as part of the json output is sent to stderr (so that includes logger messages and errors raised) and any user looking at logs might not be able to see the stdout output. This is intentional because when there is an error, it's better to notify the user twice about the error than not at all, which is what would happen, if the error is only shown through the raise then the application getting the piped stdout it will simply get an EOF instead of a json object and if the error isn't raised in favor of outputting the json object on stdout then a user looking at logs won't see any errors displayed on stderr. Hopefully this makes sense.

    2. Makes perfect sense, thanks!

  3. rbtools/commands/install.py (Diff revision 13)
     
     

    Same question here.

  4. rbtools/commands/install.py (Diff revision 13)
     
     

    And here. And many more below.

  5. 
      
david
Review request changed
Status:
Discarded