-
Notifications
You must be signed in to change notification settings - Fork 1
github_annotation: Refactor shebang from bash to python #4
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
github_annotation: Refactor shebang from bash to python #4
Conversation
Signed-off-by: Jerens Lensun <[email protected]>
Signed-off-by: Jerens Lensun <[email protected]>
Signed-off-by: Jerens Lensun <[email protected]>
Signed-off-by: Jerens Lensun <[email protected]>
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.
Looks great. I have a few minor comments:
result = emit_annotation_for_clippy(clippy_results, with_annotation) | ||
sys.exit(result) |
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.
I think you can simplify this a bit, by only parsing annotations when with_annotation
is true and then returning the original return code:
result = emit_annotation_for_clippy(clippy_results, with_annotation) | |
sys.exit(result) | |
if with_annotation: | |
emit_annotation_for_clippy(clippy_results) | |
sys.exit(proc.returncode) |
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.
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.
Yeah, clippy will not return a failure code unless -D warning
is passed, but then that will cause clippy to error just after the first warning. It is more useful in the with_annotation=true case to not rely on the exit code (we can show up to 10 warnings), so to me it makes sense to use that same logic even when GH annotation is turned off.
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.
Makes sense. Thank you!
Signed-off-by: Jerens Lensun <[email protected]>
from typing import Any | ||
|
||
|
||
def emit_annotation_for_clippy(results: list[dict[str, Any]], with_annotation: bool): |
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.
def emit_annotation_for_clippy(results: list[dict[str, Any]], with_annotation: bool): | |
def parse_cargo_output(results: list[dict[str, Any]], emit_annotation: bool) -> bool: |
) | ||
total_count += 1 | ||
|
||
return 1 if total_count > 0 else 0 |
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.
return 1 if total_count > 0 else 0 | |
return total_count > 0 |
if "--message-format=json" not in cmd: | ||
cmd.append("--message-format=json") | ||
|
||
proc = subprocess.run(cmd, capture_output=with_annotation) |
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.
If the idea is to parse the output of cargo to determine the exit status, then shouldn't we always capture the output, regardless of with_annotation
? With the current logic, when with_annotation=False
, we will not fail the build even when clippy has some output.
I tested here but this also won't fix this path issue in windows CI
Tested here: https://github.com/jerensl/ipc-channel/actions/runs/17154187746/job/48667048523?pr=2#step:5:22