Add --json flag to format rbt output as json.
Review Request #9245 — Created Oct. 3, 2017 and discarded
Information | |
---|---|
bleblan2 | |
RBTools | |
release-0.7.x | |
Reviewers | |
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 allrbt
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 … |
|
|
We should use six.text_type() instead of str() for future Python 3 compatibility. Here and lots of other places throughout your … |
|
|
Your output in this command is a little inconsistent. Here you have file_path as the key but below you use … |
|
|
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 … |
|
|
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 … |
|
|
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 … |
|
|
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. … |
|
|
Instead of defining this as a variable and then manipulating it throughout, because we only have two cases, how about … |
|
|
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 … |
|
|
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 … |
|
|
su before sy. |
|
|
We generally document these like this: Returns: dict: A dictionary with the following keys: ``success``: A boolean value for whether … |
|
|
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 … |
|

Change Summary:
Add json output formatting to more rbt subcommands.
Testing Done: |
|
||||
---|---|---|---|---|---|
Commit: |
|
||||
Diff: |
Revision 2 (+334 -86) |
Checks run (2 succeeded)

Change Summary:
Add json formatted error output to some rbt commands.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+504 -100) |
Checks run (2 succeeded)

Change Summary:
Add json formatted error output to more rbt commands.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+739 -117) |
Checks run (2 succeeded)
-
-
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']))
-
rbtools/commands/api_get.py (Diff revision 4) We should use
six.text_type()
instead ofstr()
for future Python 3 compatibility. Here and lots of other places throughout your change. -
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 usepath_to_file
. -
rbtools/commands/login.py (Diff revision 4) I suspect we want to change these uses of
logging.info()
to beprint()
like the other commands. -
rbtools/commands/logout.py (Diff revision 4) I suspect we want to change these uses of
logging.info()
to beprint()
like the other commands. -
-
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 aprint()
statement we should probably collect the information about what happened and then only print at the very end. -
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?
-
rbtools/commands/status.py (Diff revision 4) }
should be on a separate line from thefor ...
. We should also add a trailing comma after the]
-
rbtools/utils/console.py (Diff revision 4) It would be nice to pass in the
indent
parameter tojson.dumps
so that we get easier-to-read output.

Change Summary:
Change
str()
tosix.text_type()
for python 3 compatibility and formatting fixes.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+766 -128) |
Checks run (2 succeeded)

Change Summary:
Remove json formatting of warning messages in
rbt post
and changeprint()
tologging.warning()
.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+733 -129) |
Checks run (2 succeeded)

Change Summary:
Clean up inconsistencies in review request and update confirm to use stderr.
Description: |
|
|||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
|||||||||||||||||||||||||||
Diff: |
Revision 7 (+761 -136) |
Checks run (2 succeeded)

Change Summary:
Update
api-get
to only return api response when passed--json
flag and fixclear-cache
.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+749 -138) |
Checks run (2 succeeded)
-
-
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 insiderbtools/commands/*.py
-
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 thenrbtools/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. -
-
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.

Change Summary:
Move output printing into commands scripts and fix docstring for
print_json
based on review feedback.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 9 (+768 -143) |
Checks run (2 succeeded)
-
-
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. -
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), }
-
-
-
rbtools/clients/__init__.py (Diff revision 9) Kind of silly to define a variable and immediately return it. Lets just do
return [ ... ]
-
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, })
-
-
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)
-
-
rbtools/commands/patch.py (Diff revision 9) Anywhere we're using msg_input here should have a conditional with separate strings.
-
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))
-

Change Summary:
Update review with changes requested.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 10 (+804 -142) |
Checks run (2 succeeded)

Change Summary:
Replace line continuation backslashes for brackets around strings.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 11 (+804 -142) |
Checks run (2 succeeded)
-
Just a few more nits to pick.
-
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.
-
rbtools/clients/__init__.py (Diff revision 11) Should probably be "List of dicts, each containing
name
,tool_name
, anddetected
fields." -
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 beTrue
. -
rbtools/commands/patch.py (Diff revision 11) Can use
json_output_dict.extend
to do this all in one go. -
rbtools/commands/patch.py (Diff revision 11) Can use
json_output_dict.extend
to do this all in one go. -
-

Change Summary:
Fix documentation and code style based on review feedback.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 12 (+824 -143) |
Checks run (2 succeeded)
-
-
rbtools/api/cache.py (Diff revision 12) Type should be
unicode
, notstring
(string isn't a python type) -
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.

Change Summary:
Fix documentation formatting.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 13 (+825 -143) |
Checks run (2 succeeded)

-
-
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? -
-