Conversation
* Some pep8 cleanups * use argparse so that user gets a nice help message instead of silently failing
There was a problem hiding this comment.
Pull Request Overview
This PR improves the user experience by adding proper command-line argument handling with argparse and implements PEP8 code style improvements. The main goal is to replace silent failures with helpful error messages when users run the tool without proper arguments.
- Added argparse for proper command-line argument parsing with help messages
- Implemented PEP8 style improvements including quote consistency and formatting
- Refactored SBOM loading and error handling for better user feedback
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| with tempfile.NamedTemporaryFile( | ||
| delete=False, mode="w", suffix=".json" | ||
| ) as temp_file: | ||
| json.dump(self.sbom_file, temp_file) |
There was a problem hiding this comment.
This line is trying to dump self.sbom_file which is now a JSON object (loaded from load_sbom) instead of the original file path. This will cause the grype command to fail since it expects an SBOM JSON structure, not a file object.
| text=True | ||
| text=True, | ||
| ) | ||
|
|
There was a problem hiding this comment.
This line still references self.sbom_file as if it were a file path, but it's now a JSON object. The grype command expects a file path, not a JSON object.
| try: | ||
| return json.load(file) | ||
| except json.JSONDecodeError as exc: | ||
| sys.exit(f"Error reading SBOM from stdin: {exc}") |
There was a problem hiding this comment.
The error message mentions 'stdin' but the function is actually loading from a file argument, not stdin. The message should be updated to reflect that it's reading from the provided file.
| sys.exit(f"Error reading SBOM from stdin: {exc}") | |
| sys.exit(f"Error reading SBOM from file '{file.name}': {exc}") |
|
Hi Paul, Thanks so much for taking the time to submit this PR! I really appreciate the suggestions, especially around using argparse for better CLI handling. The codebase has evolved quite a bit since you submitted this in February, and I've already incorporated several of your improvements:
I've just created PR #25 that refactors the argument parsing to use argparse as you suggested, with credit to you in the commit message. Rather than asking you to rebase (which would be painful given how much has changed), I thought it made more sense to cherry-pick the valuable ideas from your PR. Regarding the stdin streaming for JSON - I agree it's tricky with the TUI needing stdin. For now, requiring a file path keeps things simple and predictable. Thanks again for the contribution! I'm closing this PR since the improvements are now incorporated in #25, but your input definitely made grummage better. |
Hi Alan! not sure if you are interested in contributions for this, but I noticed that when you run it with no args that you get nothing back. So I added argparse, along with a few other cleanups and such, so that you get a help message in case you forget to specify args.
I noticed that it looks like you were also trying to add support for streaming a json file to it through a pipe, but it didn't look functional yet. I tried see if I could get it working, but it seems to get confused, since stdin is reading from the file pipe and then later needs to read from the tty for the TUI. Didn't sort out a fix yet so I skipped it for now, since there's no loss in functionality, but if it's critical then I could possibly take a stab at it again later.