-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move parsing into parse
subcommand (introduce argparse)
#25
Conversation
"Parse" is a better reflection of what this function does.
We intend to add more functionality to this tool, so it no longer makes sense to have the error parsing functionality at the top level when calling the script. This change introduces (and requires) a new "parse" subcommand, under which the existing parsing function is moved. Because we're using argparse to do this, we also get a new "--help" flag on the command.
9b00149
to
74319ef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This broadly looks good! I had a go at adding an argument for specifying the level of indentation used in the JSON output, just to test this out, and it was very easy:
diff --git a/mypy_json_report.py b/mypy_json_report.py
index f7db976..1163d28 100644
--- a/mypy_json_report.py
+++ b/mypy_json_report.py
@@ -42,6 +42,7 @@ def main() -> None:
)
parse_parser.set_defaults(func=_parse_command)
+ parse_parser.add_argument("-i", "--indentation", type=int, default=2, help="Number of spaces to indent JSON output")
args = sys.argv[1:]
parsed = parser.parse_args(args)
@@ -51,7 +52,7 @@ def main() -> None:
def _parse_command(args: object) -> None:
"""Handle the `parse` command."""
errors = parse_errors_report(sys.stdin)
- error_json = json.dumps(errors, sort_keys=True, indent=2)
+ error_json = json.dumps(errors, sort_keys=True, indent=args.indentation)
print(error_json)
Co-authored-by: Jeppe Fihl-Pearson <[email protected]>
The summary returned is no longer JSON. Co-authored-by: Jeppe Fihl-Pearson <[email protected]>
There's no point in passing in the defaults. Co-authored-by: Jeppe Fihl-Pearson <[email protected]>
Thanks! The recommendation to use I really like the idea of this indentation flag -- it'll make things easier in environments with CI or pre-commit hooks that have opinions on indentation. I'll add that into this PR in a sec. |
Some environments may want control over this to make working with existing style-guides easier. Co-authored-by: Jeppe Fihl-Pearson <[email protected]>
5bb967c
to
2ddb4b8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thanks! I've got a couple more PRs to open still, so I'm not going to push a release tag yet, but I'll merge this now to make them easier. |
We intend to add more functionality to this tool, so it no longer makes sense to have the error parsing functionality at the top level when calling the script.
This change introduces (and requires) a new
parse
subcommand, under which the existing parsing function is moved. This change is backwards incompatible.Because we're using argparse to do this, we also get a new "--help" flag on the command.
We also add a new
--indentation
flag to theparse
subcommand which grants control over the indentation in the JSON output.This is spun out of #18, which will introduce a new command.