Add submission_info command#6
Conversation
if script is running in Cygwin environment, os.path.abspath will broke Windows path Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
Summary of ChangesHello @kostyanf14, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement to the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new submission_info command, which is a useful addition. The implementation is generally sound, but I've identified several areas for improvement to enhance code quality, maintainability, and user experience. My feedback includes suggestions to refactor duplicated code, correct function signatures for consistency, fix incorrect error messages, handle exceptions more specifically, and refactor repetitive code blocks. Addressing these points will make the new feature more robust and the overall codebase cleaner.
| input_json_data = None | ||
| if args.input_json: | ||
| try: | ||
| with open(args.input_json, "r") as f: | ||
| input_json_data = json.load(f) | ||
| print(f"[INFO] Loaded input JSON data from {args.input_json}") | ||
| except Exception as e: | ||
| print(f"Error: Failed to load input JSON file '{args.input_json}': {e}") | ||
| sys.exit(1) | ||
|
|
||
| if not args.product_id: | ||
| if input_json_data and "product_id" in input_json_data: | ||
| args.product_id = input_json_data["product_id"] | ||
| else: | ||
| print("Error: --product_id is required for download action") | ||
| sys.exit(1) | ||
| if not args.submission_id: | ||
| if input_json_data and "submission_id" in input_json_data: | ||
| args.submission_id = input_json_data["submission_id"] | ||
| else: | ||
| print("Error: --submission_id is required for download action") | ||
| sys.exit(1) | ||
| if not args.output_file: | ||
| if input_json_data and "output_file" in input_json_data: | ||
| args.output_file = input_json_data["output_file"] |
There was a problem hiding this comment.
This block of code for handling arguments from a JSON file is nearly identical to the one in main_wait_download. To reduce code duplication and improve maintainability, consider extracting this logic into a helper function. The helper could take args and a list of required arguments as parameters, and return the populated args object. This would make the code more DRY (Don't Repeat Yourself).
| except Exception as e: | ||
| print(f"Error: Failed to load input JSON file '{args.input_json}': {e}") | ||
| sys.exit(1) |
There was a problem hiding this comment.
Catching a broad Exception is generally discouraged as it can hide unexpected errors. It's better to catch more specific exceptions that you expect to handle, such as FileNotFoundError and json.JSONDecodeError.
| except Exception as e: | |
| print(f"Error: Failed to load input JSON file '{args.input_json}': {e}") | |
| sys.exit(1) | |
| except (FileNotFoundError, json.JSONDecodeError) as e: | |
| print(f"Error: Failed to load or parse input JSON file '{args.input_json}': {e}") | |
| sys.exit(1) |
| match = re.search(r"\s*commitStatus:\s*(\S+)", text_info) | ||
| if match: | ||
| results["commit_status"] = match.group(1) | ||
| match = re.search(r"\s*Name:\s*(\S+.*)", text_info) | ||
| if match: | ||
| results["name"] = match.group(1) | ||
| match = re.search(r"\s*type:\s*(\S+)", text_info) | ||
| if match: | ||
| results["type"] = match.group(1) | ||
| match = re.search(r"\s*> Step:\s*(\S+)", text_info) | ||
| if match: | ||
| results["step"] = match.group(1) | ||
| match = re.search(r"\s*> State:\s*(\S+)", text_info) | ||
| if match: | ||
| results["state"] = match.group(1) |
There was a problem hiding this comment.
The code uses a series of re.search calls and if match: blocks to parse the text output. This pattern is repetitive. You could refactor this into a more data-driven approach by using a list of tuples, where each tuple contains the result key and the regex pattern. This would make the code more concise and easier to extend if more fields need to be parsed in the future.
| match = re.search(r"\s*commitStatus:\s*(\S+)", text_info) | |
| if match: | |
| results["commit_status"] = match.group(1) | |
| match = re.search(r"\s*Name:\s*(\S+.*)", text_info) | |
| if match: | |
| results["name"] = match.group(1) | |
| match = re.search(r"\s*type:\s*(\S+)", text_info) | |
| if match: | |
| results["type"] = match.group(1) | |
| match = re.search(r"\s*> Step:\s*(\S+)", text_info) | |
| if match: | |
| results["step"] = match.group(1) | |
| match = re.search(r"\s*> State:\s*(\S+)", text_info) | |
| if match: | |
| results["state"] = match.group(1) | |
| parsing_rules = [ | |
| ("commit_status", r"\s*commitStatus:\s*(\S+)"), | |
| ("name", r"\s*Name:\s*(\S+.*)"), | |
| ("type", r"\s*type:\s*(\S+)"), | |
| ("step", r"\s*> Step:\s*(\S+)"), | |
| ("state", r"\s*> State:\s*(\S+)"), | |
| ] | |
| for key, pattern in parsing_rules: | |
| match = re.search(pattern, text_info) | |
| if match: | |
| results[key] = match.group(1) |
kostyanf14
force-pushed
the
info_cmd
branch
2 times, most recently
from
2e88056 to
d510c3e
Compare
Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
|
LGTM test with: python3 hw_submission_automation.py submission_info --product_id=13897625692316071 --submission_id=1152921505700117129{ |
2 participants
No description provided.