Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd --reporter parameter #10
Conversation
sunshinejr
added some commits
Feb 18, 2018
sunshinejr
force-pushed the
sunshinejr:feature/reporter
branch
from
4e2542e
to
cf9c19c
Feb 18, 2018
sunshinejr
force-pushed the
sunshinejr:feature/reporter
branch
from
cf9c19c
to
e978b30
Feb 18, 2018
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Hey @sunshinejr, sorry for the delay. I've missed your pull request originally, so thanks for tagging me. We also use Danger for the Lyft iOS apps, so your change is very welcome. By the way, the so-called "raw" output was actually designed for ingest by another one of our tools, https://github.com/lyft/linty_fresh. Let me review your code and hopefully merge it. |
| "file": self.path, | ||
| "line": element.line, | ||
| "error": new_error, | ||
| "rule": self.rule_name |
This comment has been minimized.
This comment has been minimized.
| error_dict["file"], | ||
| error_dict["line"], | ||
| error_dict["error"], | ||
| error_dict["rule"] |
This comment has been minimized.
This comment has been minimized.
| for path in config.include_paths: | ||
| for root, _, files in os.walk(path): | ||
| for file_path in [os.path.join(root, file) for file in files]: | ||
| checkers = config.checkers(file_path) | ||
| success = process_file(file_path, checkers) and success | ||
| errors = errors + process_file(file_path, checkers, reporter) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| parser.parse_args() | ||
| parser.add_argument("--reporter", choices=("raw", "json"), | ||
| default="raw", | ||
| help="custom reporter to use (optional)") |
This comment has been minimized.
This comment has been minimized.
ikonst
Apr 16, 2018
Contributor
argparse arguments are always optional, so perhaps we can omit the "(optional)". The default value "raw" would be indicated as part of the help text anyway.
| @@ -3,6 +3,7 @@ | |||
| import argparse | |||
| import os | |||
| import sys | |||
| import json | |||
This comment has been minimized.
This comment has been minimized.
| default="raw", | ||
| help="custom reporter to use (optional)") | ||
| args = parser.parse_args() | ||
| reporter = args.reporter |
This comment has been minimized.
This comment has been minimized.
ikonst
Apr 16, 2018
Contributor
After you assign this variable, you're never changing it, so perhaps directly using args.reporter will make it clearer that it's coming from the args.
|
|
||
| sys.exit(0 if errors.count == 0 else 1) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
sunshinejr
Apr 16, 2018
Contributor
I think I couldn't get this one right and was hoping that you could help me with it in a PR but forgot to comment it
| message.format(*args), | ||
| self.rule_name, | ||
| )) | ||
| message.format(*args) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@ikonst thanks for the review! I've added a commit with suggested fixes and tested it locally - it still works |
|
|
||
| sys.exit(len(errors) == 0) |
This comment has been minimized.
This comment has been minimized.
ikonst
Apr 16, 2018
•
Contributor
That looks incorrect. You want to exit with zero for success and non-zero for failure. You can also simply say:
sys.exit(1 if errors else 0)
This comment has been minimized.
This comment has been minimized.
sunshinejr
Apr 16, 2018
•
Contributor
Oh, my mistake. Was so ashamed of this line that I quickly copied/pasted this one and wanted to forget about it
ikonst
merged commit 8405d84
into
lyft:master
Apr 16, 2018
This comment has been minimized.
This comment has been minimized.
|
Thanks again @sunshinejr. Excited to see this is useful outside of our company! |
sunshinejr commentedFeb 18, 2018
Adds optional reporter parameter with (currently) two options: "raw" and "json". Raw is the current one, while "json" returns a json string (useful to libraries using the output of linters, e.g. Danger).
As there is no doc around contributing to this repo: If this is something you don't want to see, just let me know and I can work on my own fork. Cheers!