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

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

bleblan2
RBTools
release-0.7.x
rbtools, students

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)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    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)
     
     

    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)
     
     

    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)
     
     

    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)
     
     

    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)
     
     

    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)
     
     
     
     

    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)
     
     

    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)
     
     
     

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

    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)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    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)
     
     

    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)
     
     
     
     
     
     
     
     
     
     
     

    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)
     
     

    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)
     
     
     

    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)
     
     
     
     
     

    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)
     
     

    Perhaps we want to rename this method now?

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

    Should use the standard Args/Returns sections.

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

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

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

    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)
     
     
     

    Same here re: dict.extend

  9. rbtools/commands/logout.py (Diff revision 9)
     
     
     
     
     
     
     

    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)
     
     
     

    Same here with msg_input not being localizable.

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

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

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

    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)
     
     
     

    su before sy.

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

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

    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)
     
     
     

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

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

    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)
     
     
     

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

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

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

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

    And here.

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

    And finally here.

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

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

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

    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

Loading...