Skip to content
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

Merged
merged 9 commits into from
Feb 3, 2023

Conversation

meshy
Copy link
Collaborator

@meshy meshy commented Jan 31, 2023

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 the parse subcommand which grants control over the indentation in the JSON output.

This is spun out of #18, which will introduce a new command.

@meshy meshy requested a review from Tenzer January 31, 2023 15:54
mypy_json_report.py Outdated Show resolved Hide resolved
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.
@meshy meshy force-pushed the switch-to-argparse branch from 9b00149 to 74319ef Compare January 31, 2023 18:11
Copy link
Member

@Tenzer Tenzer left a 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)

mypy_json_report.py Outdated Show resolved Hide resolved
mypy_json_report.py Outdated Show resolved Hide resolved
mypy_json_report.py Outdated Show resolved Hide resolved
meshy and others added 3 commits January 31, 2023 22:30
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]>
@meshy
Copy link
Collaborator Author

meshy commented Jan 31, 2023

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:

Thanks! The recommendation to use argparse was a good one, for sure.

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]>
@meshy meshy force-pushed the switch-to-argparse branch from 5bb967c to 2ddb4b8 Compare January 31, 2023 22:56
Copy link
Member

@Tenzer Tenzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@meshy
Copy link
Collaborator Author

meshy commented Feb 3, 2023

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.

@meshy meshy merged commit 0328ce0 into main Feb 3, 2023
@meshy meshy deleted the switch-to-argparse branch February 3, 2023 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants