chore: configure ruff lint rules and fix what they surfaced - #883
chore: configure ruff lint rules and fix what they surfaced#883maxwbuckley wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe pull request adds project-wide Ruff configuration and applies related Python cleanup. Changes include explicit class-variable and optional-argument typing, abstract-method simplification, unused-variable cleanup, parameterized logging, exception chaining, subprocess return-code handling, and equivalent iteration changes. Tests update exception types and regular-expression patterns. Camera frame output and other existing behavior remain unchanged. Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
📝 Docs preview is not auto-deployed for fork PRs. A maintainer with write access to |
60dd35f to
69137c8
Compare
|
Hey we quickly looked over this, general feel is good. What are you feeling about merging this? |
The ruff and ruff-format pre-commit hooks were running with no configuration
at all, so only ruff's built-in defaults (E4/E7/E9 + F) were ever enforced.
Adds a [tool.ruff] section selecting the correctness-oriented groups:
B, C4, PIE, PERF, PLE, PLW, LOG, G, ASYNC and RUF. Formatting is unchanged
(ruff-format defaults, which the tree already matches).
Everything in `select` is currently clean, so the hook is enforceable as-is.
Each entry in `ignore` is a deliberate call with its reason inline; the
notable ones:
- B905 (zip strict=) is a per-call-site behaviour change, turning a silent
truncation into a runtime exception. Worth adopting deliberately, not in a
lint sweep.
- RUF005, C408 and RUF007 are style-only rewrites of code that is already
correct and readable (`a + [b]` -> `[*a, b]`, dict(a=1) -> {"a": 1},
zip(x[:-1], x[1:]) -> itertools.pairwise(x)). Enforcing them means churning
working call sites for no behaviour change, so they are left to author
preference.
- RUF022 sorts __all__ alphabetically, which scrambles the semantic grouping
comments the schema/package __init__ files rely on.
- RUF100 is evaluated against `select`, so it flags every noqa written for a
rule not yet enabled (BLE001, PLC0415, N803, ...). Re-enable once those
groups are adopted.
No live defects were found. Every rule fired on code that behaves correctly
today; what follows removes fragility, not bugs.
Correct today, fragile to a later change:
- LOG014: _record_error passes exc_info=True, which reads the ambient
sys.exc_info(). Both of its callers invoke it from inside an except block,
so the traceback is logged correctly; the rule is lexical and fires because
the logging call sits in a helper rather than in the handler itself.
Passing the exception explicitly makes it independent of the caller's
context.
- B023: a closure in the frame loop captured the loop variable `phase` by
reference. It is called immediately in the same iteration, so the value was
always correct. `fill` is hoisted above the loop and takes `phase` as a
parameter, which also stops re-creating the function object every frame.
Binding it as a default argument would satisfy the rule equally.
- B011: `assert False` in a test, which python -O strips, turning a failure
into a silent pass. The suite is not run under -O today. Replaced by calling
the constructor directly, so an exception fails the test with its own
traceback.
- B017: a blind pytest.raises(Exception) that would also accept an unrelated
failure. The test passes for the right reason today; naming the accepted
exception set keeps it that way.
- RUF043: pytest match="libcloudxr.so" treats '.' as a regex wildcard where a
literal filename was meant. The real message contains the literal, so the
assertion passes correctly, but it is weaker than it reads. The remaining
patterns are intentional regexes and are now raw strings.
Typing and explicitness:
- RUF012: three mutable class attributes annotated ClassVar, one of them on
the EnvConfig singleton.
- RUF013: implicit Optional spelled out as `str | None`.
- B904: `raise ... from` on re-raises, so the original cause is not lost.
- G004: log calls take %s arguments rather than eagerly formatted f-strings.
- PLW1510: subprocess.run calls that inspect returncode say check=False.
The tree already spelled this out at 10 call sites, 8 of them in
oob_teleop_adb.py; this covers the stragglers.
TRY004 (ValueError -> TypeError in TeleopSessionConfig validation) was left
alone: it is a public API behaviour change, not a lint fix.
Verified on Ubuntu 24.04 / Python 3.12: ruff check and ruff format --check
both clean at v0.15.1, the version pinned in .pre-commit-config.yaml, and
SKIP=check-copyright-year pre-commit run --all-files passes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Max Buckley <maxwbuckley@gmail.com>
69137c8 to
13e7dca
Compare
|
Thanks for looking! I'm happy to merge this — it's rebased onto current One thing worth calling out from the rebase. The only conflict was in Re-verified after the rebase, at the ruff version pinned in
Which is the ask: the workflow runs are all sitting at |
@jiwenc-nv this is okay right? |
3 participants
Description
The
ruffandruff-formatpre-commit hooks were running with no configuration atall, so only ruff's built-in defaults (E4/E7/E9 + F) were ever enforced. Adds a
[tool.ruff]section selecting the correctness-oriented groups: B, C4, PIE, PERF,PLE, PLW, LOG, G, ASYNC and RUF. Formatting is unchanged (ruff-format defaults, which
the tree already matches).
Everything in
selectis currently clean, so the hook is enforceable as-is. Each entryin
ignoreis a deliberate call with its reason inline; the notable ones:B905(zipstrict=) is a per-call-site behaviour change, turning a silenttruncation into a runtime exception. Worth adopting deliberately, not in a lint sweep.
RUF005,C408andRUF007are style-only rewrites of code that is already correctand readable (
a + [b]→[*a, b],dict(a=1)→{"a": 1},zip(x[:-1], x[1:])→itertools.pairwise(x)). Enforcing them means churning workingcall sites for no behaviour change, so they are left to author preference.
RUF022sorts__all__alphabetically, which scrambles the semantic groupingcomments the schema/package
__init__files rely on.RUF100is evaluated againstselect, so it flags everynoqawritten for a rule notyet enabled (
BLE001,PLC0415,N803, ...). Re-enable once those groups are adopted.No live defects were found. Every rule fired on code that behaves correctly today;
what follows removes fragility, not bugs.
Correct today, fragile to a later change
LOG014:_record_errorpassesexc_info=True, which reads the ambientsys.exc_info(). Both of its callers invoke it from inside anexceptblock, so thetraceback is logged correctly; the rule is lexical and fires because the logging call
sits in a helper rather than in the handler itself. Passing the exception explicitly
makes it independent of the caller's context.
B023: a closure in the frame loop captured the loop variablephaseby reference.It is called immediately in the same iteration, so the value was always correct.
fillis hoisted above the loop and takesphaseas a parameter, which also stopsre-creating the function object every frame. Binding it as a default argument would
satisfy the rule equally.
B011:assert Falsein a test, whichpython -Ostrips, turning a failure into asilent pass. The suite is not run under
-Otoday. Replaced by calling theconstructor directly, so an exception fails the test with its own traceback.
B017: a blindpytest.raises(Exception)that would also accept an unrelatedfailure. The test passes for the right reason today; naming the accepted exception set
keeps it that way.
RUF043: pytestmatch="libcloudxr.so"treats.as a regex wildcard where a literalfilename was meant. The real message contains the literal, so the assertion passes
correctly, but it is weaker than it reads. The remaining patterns are intentional
regexes and are now raw strings.
Typing and explicitness
RUF012: three mutable class attributes annotatedClassVar, one of them on theEnvConfigsingleton.RUF013: implicit Optional spelled out asstr | None.B904:raise ... fromon re-raises, so the original cause is not lost.G004: log calls take%sarguments rather than eagerly formatted f-strings.PLW1510:subprocess.runcalls that inspectreturncodesaycheck=False. Thetree already spelled this out at 10 call sites, 8 of them in
oob_teleop_adb.py; thiscovers the stragglers.
TRY004(ValueError→TypeErrorinTeleopSessionConfigvalidation) was leftalone: it is a public API behaviour change, not a lint fix.
Python only — no C++ or CMake is touched. A companion PR adds the C++ warning set; the
two are independent and can land in either order.
Type of change
Testing
Ubuntu 24.04 / x86_64, Python 3.12 —
ruff checkandruff format --checkboth clean at v0.15.1, the version pinned in.pre-commit-config.yaml(268 files).SKIP=check-copyright-year pre-commit run --all-files: all hooks pass.ctest: 309/310. The one failure,cloudxr_test_launcher, is the missing CloudXR SDK(no NGC key on this host, so the download 404s and
get_sdk_path()raises) — anenvironment gap, not a code failure.
Forward-compat note, not addressed here: at ruff 0.16
format --checkwants to reflowPython code blocks embedded in three Markdown files (
examples/teleop_ros2/README.md,src/plugins/oak/README.md, and one other) — newer ruff formats fenced code in Markdown,which 0.15.1 does not. None of those files are touched by this PR, but it will surface
whenever the pre-commit rev is bumped past 0.16.
Checklist
SKIP=check-copyright-year pre-commit run --all-filesgit commit -s) per the DCODocumentation: no user-facing behaviour changes, so no doc updates. Every non-obvious
ignoreentry is documented inline inpyproject.toml.Tests: no new tests — this is a lint-configuration change plus the fixes it surfaced,
and it is exercised by the existing suite passing.
Summary by CodeRabbit
Bug Fixes
Quality Improvements