-
Notifications
You must be signed in to change notification settings - Fork 4
updates in handling github event #19
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,23 +1,42 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import os | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import json | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import sys | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from typing import Optional, Any | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def load_github_event(event_path: str) -> Optional[Any]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def load_github_event(event_path: str) -> Optional[Any]: | |
| def load_github_event(event_path: str) -> Optional[dict]: |
Standards
- Type Safety
- PEP 484
arvi18 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
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.
Suggestion: The duplicated try blocks in the event loader mean that the file read and JSON parse happen in the first try without any attached except, so any FileNotFoundError or JSONDecodeError will bypass your error handlers and crash the script instead of being caught and converted to a graceful None return. [logic error]
Severity Level: Minor
| try: | |
| with open(event_path, "r") as file: | |
| return json.load(file) |
Why it matters? ⭐
The PR's final file contains a duplicated try: block: the first try has no except so any FileNotFoundError/JSONDecodeError raised there will skip the following handlers and crash. The improved code removes the stray first try so exceptions are actually caught and the function can gracefully return None. This fixes a real logic/exception-handling bug introduced by the PR.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** main.py
**Line:** 19:21
**Comment:**
*Logic Error: The duplicated `try` blocks in the event loader mean that the file read and JSON parse happen in the first `try` without any attached `except`, so any `FileNotFoundError` or `JSONDecodeError` will bypass your error handlers and crash the script instead of being caught and converted to a graceful `None` return.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Outdated
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 function is well-structured for loading the event data. For better adherence to command-line tool conventions, error messages should be printed to standard error (sys.stderr) rather than standard output. This allows users to separate normal output from error logs. You can do this by adding the file=sys.stderr argument to your print calls within the except blocks.
| def load_github_event(event_path: str) -> Optional[Any]: | |
| try: | |
| with open(event_path, "r") as file: | |
| return json.load(file) | |
| except FileNotFoundError: | |
| print(f"❌ Event file not found: {event_path}") | |
| except json.JSONDecodeError as e: | |
| print(f"❌ Failed to parse JSON: {e}") | |
| except Exception as e: | |
| print(f"❌ Unexpected error reading event file: {e}") | |
| return None | |
| def load_github_event(event_path: str) -> Optional[Any]: | |
| try: | |
| with open(event_path, "r") as file: | |
| return json.load(file) | |
| except FileNotFoundError: | |
| print(f"❌ Event file not found: {event_path}", file=sys.stderr) | |
| except json.JSONDecodeError as e: | |
| print(f"❌ Failed to parse JSON: {e}", file=sys.stderr) | |
| except Exception as e: | |
| print(f"❌ Unexpected error reading event file: {e}", file=sys.stderr) | |
| return None |
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.
Similar to the error handling in load_github_event, this warning and error message should also be directed to standard error (sys.stderr). This is a standard practice for command-line applications to distinguish between informational output and diagnostics.
| if not github_event_name: | |
| print("⚠️ GITHUB_EVENT_NAME not set.") | |
| else: | |
| print(f"📦 Received GitHub event: {github_event_name}") | |
| if not github_event_path: | |
| print("GITHUB_EVENT_PATH not set, cannot read event data.") | |
| return | |
| print("❌ GITHUB_EVENT_PATH not set. Cannot read event data.") | |
| sys.exit(1) | |
| if not github_event_name: | |
| print("⚠️ GITHUB_EVENT_NAME not set.", file=sys.stderr) | |
| else: | |
| print(f"📦 Received GitHub event: {github_event_name}") | |
| if not github_event_path: | |
| print("❌ GITHUB_EVENT_PATH not set. Cannot read event data.", file=sys.stderr) | |
| sys.exit(1) |
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.
Inconsistent Error Handling
Inconsistent error handling between github_event_name and github_event_path. Missing event name only prints a warning while missing event path exits the program, creating unpredictable behavior.
| if not github_event_name: | |
| print("⚠️ GITHUB_EVENT_NAME not set.") | |
| else: | |
| print(f"📦 Received GitHub event: {github_event_name}") | |
| if not github_event_path: | |
| print("GITHUB_EVENT_PATH not set, cannot read event data.") | |
| return | |
| print("❌ GITHUB_EVENT_PATH not set. Cannot read event data.") | |
| sys.exit(1) | |
| if not github_event_name: | |
| print("❌ GITHUB_EVENT_NAME not set. Cannot process event.") | |
| sys.exit(1) | |
| print(f"📦 Received GitHub event: {github_event_name}") | |
| if not github_event_path: | |
| print("❌ GITHUB_EVENT_PATH not set. Cannot read event data.") | |
| sys.exit(1) |
Standards
- Consistent Error Handling
- Fail Fast Principle
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.
Missing Newline
The file is missing a newline at the end, which is causing the SECTION 1 text to be appended to the code. This could cause syntax errors or unexpected behavior.
| main() | |
| main() |
Standards
- PEP 8
- File Format Standards
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.
Suggestion: The imported
Anytype fromtypingis never used, which is dead code and can cause unnecessary confusion about intended type usage in this module. [code quality]Severity Level: Minor⚠️
Why it matters? ⭐
The final file never references
Any. Removing the unused import is a small, correct cleanup (reduces linter noise and clarifies intent). It's a code-quality change, not a functional fix, but it's valid and safe.Prompt for AI Agent 🤖