-
Notifications
You must be signed in to change notification settings - Fork 37
Add Sourcery CI workflow #948
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
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds Sourcery static analysis to the project via pre-commit and small refactors suggested by Sourcery in qmctl, along with cleanup of an unnecessary pass statement. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
99a8707 to
36c2ec7
Compare
|
@sourcery-ai review |
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.
|
Please take a look here https://github.com/containers/qm/actions/runs/19296341436/job/55179130811?pr=948 There are output and ideas to fix |
a3c713b to
6b6e6e0
Compare
f8f28be to
0573ac4
Compare
398415e to
6c2a7b5
Compare
1eef217 to
c1f445b
Compare
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.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tools/qmctl/qmctl.py:264-269` </location>
<code_context>
- options = kwargs if kwargs else defaults
+ options = kwargs or defaults
try:
- result = subprocess.run( # nosec B603
+ return subprocess.run( # nosec B603
cmd,
**options,
)
- return result
except Exception as e:
raise RuntimeError(f"subprocess.run failed: {e}") from e
</code_context>
<issue_to_address>
**issue (bug_risk):** Narrow the exception type around `subprocess.run` to avoid swallowing interrupts and unrelated errors.
Catching `Exception` here will also intercept `KeyboardInterrupt` and `SystemExit`, making the CLI harder to interrupt and potentially hiding unrelated problems. Prefer catching specific errors (e.g., `OSError`, `subprocess.SubprocessError`) so only subprocess-related failures are wrapped in `RuntimeError` and truly exceptional conditions still propagate.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
c1f445b to
9fcf02f
Compare
9fcf02f to
b4db463
Compare
- Add Sourcery pre-commit hook with exclusions for demos and test directories - Fix syntax error in tests/qm-ipc/server/server.py closes https://issues.redhat.com/browse/VROOM-34063 Signed-off-by: Artiom Divak <adivak@redhat.com>
b4db463 to
304f2c7
Compare
Yarboa
left a comment
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.
LGTM,
Thank you
|
triggering CI/CD failure. |
|
Related: #951 |
Summary by Sourcery
resolve #940
Add a new GitHub Actions workflow to run Sourcery code reviews on pull requests.
CI:
Summary by Sourcery
Integrate Sourcery static analysis into local tooling and apply minor code cleanups to qmctl.
Enhancements: