Skip to content

chore: configure ruff lint rules and fix what they surfaced - #883

Draft
maxwbuckley wants to merge 1 commit into
NVIDIA:mainfrom
maxwbuckley:chore/ruff-lint-rules
Draft

chore: configure ruff lint rules and fix what they surfaced#883
maxwbuckley wants to merge 1 commit into
NVIDIA:mainfrom
maxwbuckley:chore/ruff-lint-rules

Conversation

@maxwbuckley

@maxwbuckley maxwbuckley commented Aug 4, 2026

Copy link
Copy Markdown

Description

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 (ValueErrorTypeError in TeleopSessionConfig validation) was left
alone: 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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Testing

Ubuntu 24.04 / x86_64, Python 3.12

  • ruff check and ruff format --check both 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) — an
    environment gap, not a code failure.

Forward-compat note, not addressed here: at ruff 0.16 format --check wants to reflow
Python 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

  • I have read and understood the contribution guidelines
  • I have run the linter and formatter with SKIP=check-copyright-year pre-commit run --all-files
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix/feature works (or explained why not)
  • I have signed off all my commits (git commit -s) per the DCO

Documentation: no user-facing behaviour changes, so no doc updates. Every non-obvious
ignore entry is documented inline in pyproject.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.

@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: e768d1f7-467f-463e-a90c-1005f02ea124

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

📝 Docs preview is not auto-deployed for fork PRs.

A maintainer with write access to NVIDIA/IsaacTeleop can deploy a preview by
commenting /preview-docs on this PR. Once deployed, the preview
will live at:

https://nvidia.github.io/IsaacTeleop/preview/pr-883/

@maxwbuckley
maxwbuckley force-pushed the chore/ruff-lint-rules branch from 093fc74 to 60dd35f Compare August 4, 2026 14:20
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants