Skip to content

Conversation

@Mayankm96
Copy link
Contributor

@Mayankm96 Mayankm96 commented Jan 4, 2026

Description

Ruff can handle linting, formatting, and type-checking (where applicable), streamlining our development workflow and improving code quality.

This PR replaces our current linting setup with Ruff. Subsequent MRs will look into using Ruff for formatting and import ordering.

Type of change

  • Documentation update

Checklist

  • I have read and understood the contribution guidelines
  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the changelog and the corresponding version in the extension's config/extension.toml file
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Jan 13, 2026
@github-actions github-actions bot added the isaac-lab Related to Isaac Lab team label Jan 13, 2026
@github-actions github-actions bot added the isaac-mimic Related to Isaac Mimic team label Jan 13, 2026
@github-actions github-actions bot added the asset New asset feature or request label Jan 13, 2026
@Mayankm96 Mayankm96 changed the title Switches code formatting and linting to Ruff Switches code linting to Ruff Jan 13, 2026
@Mayankm96 Mayankm96 marked this pull request as ready for review January 13, 2026 12:32
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 13, 2026

Greptile Overview

Greptile Summary

This PR migrates from flake8 to Ruff for Python linting, but introduces several critical issues:

Configuration Issues

Incomplete Migration: The PR claims to replace the linting setup with Ruff for "linting, formatting, and type-checking," but the actual implementation only migrates linting. The ruff-format hook is commented out in .pre-commit-config.yaml, while Black and isort remain active. This creates a confusing three-tool setup (Ruff + Black + isort) instead of the promised unified Ruff solution.

Version Inconsistency: Ruff is configured for Python 3.10 (target-version = "py310") while Black and Pyright target Python 3.11, creating potential incompatibilities in linting behavior.

Critical Logic Changes

This linting PR introduces multiple breaking changes that go beyond formatting:

  1. importer.py (CRITICAL): The refactored seen() function completely changes package traversal semantics. The original used a mutable default argument to maintain state across the entire walk, while the new code creates a fresh dictionary for each package iteration. Additionally, line 90 changes or [] to or None, which will cause a TypeError when iterating over path in line 95.

  2. spaces.py (BREAKING): Adds an explicit ValueError for unsupported Gymnasium space types instead of the previous silent None return. This will break any code that relied on the None return value.

  3. Multiple files: Comparison operators reversed (e.g., S > min_singular_valuemin_singular_value < S) by Ruff's SIM103 rule, which is explicitly ignored in the config but still applied.

Other Changes

  • Type hints modernized from Optional[T] to T | None (Python 3.10+ syntax)
  • Type comparisons changed from == to is (better practice)
  • Import reordering and formatting adjustments
  • Spelling fixes ("re-used" → "reused")
  • Added noqa comments where Ruff rules need to be suppressed

Recommendations

  1. Fix the critical bugs in importer.py before merging
  2. Document the breaking change in spaces.py
  3. Clarify the migration scope: Is this Ruff linting only, or full migration?
  4. Align Python versions across all tools (use 3.11 consistently)
  5. Consider whether logic changes belong in a linting PR

Confidence Score: 1/5

  • This PR contains critical bugs that will cause runtime errors and is NOT safe to merge without fixes
  • Score of 1/5 reflects multiple critical issues: (1) TypeError bug in importer.py line 90 that will crash on execution, (2) Breaking semantic change to package traversal logic in importer.py that fundamentally alters behavior, (3) Undocumented breaking change in spaces.py that changes error handling, (4) Inconsistent configuration with misleading PR description. While most formatting changes are safe, the logic bugs make this PR unsafe to merge as-is.
  • CRITICAL: source/isaaclab_tasks/isaaclab_tasks/utils/importer.py (will crash). HIGH PRIORITY: source/isaaclab/isaaclab/envs/utils/spaces.py (breaking change), .pre-commit-config.yaml and pyproject.toml (configuration inconsistencies)

Important Files Changed

File Analysis

Filename Score Overview
source/isaaclab_tasks/isaaclab_tasks/utils/importer.py 1/5 Critical logic changes to seen() function that alter package traversal behavior and introduce a potential TypeError
source/isaaclab/isaaclab/envs/utils/spaces.py 2/5 Breaking change: adds explicit ValueError for unsupported spaces instead of returning None
.pre-commit-config.yaml 2/5 Adds Ruff linting but ruff-format commented out; keeps Black and isort creating inconsistent setup
pyproject.toml 3/5 Adds Ruff config but keeps Black/isort sections; Python version mismatch (py310 vs py311)
source/isaaclab/isaaclab/controllers/differential_ik.py 4/5 Comparison order reversed (S > min to min < S) - logically equivalent but changed by Ruff
source/isaaclab_tasks/isaaclab_tasks/direct/automate/soft_dtw_cuda.py 4/5 Comparison order reversed and formatting changes; removes blank lines

Sequence Diagram

sequenceDiagram
    participant Dev as Developer
    participant PreCommit as Pre-commit Hooks
    participant Ruff as Ruff Linter
    participant Black as Black Formatter
    participant Isort as Isort
    participant Code as Codebase

    Dev->>PreCommit: Commit changes
    
    Note over PreCommit: Migration to Ruff
    PreCommit->>Ruff: Run ruff --fix
    Ruff->>Code: Apply linting fixes
    Note right of Ruff: UP (pyupgrade)<br/>SIM (simplify)<br/>RET (return)<br/>F (pyflakes)<br/>E/W (pycodestyle)
    
    Note over PreCommit: Still using Black + isort
    PreCommit->>Black: Format code
    Black->>Code: Apply formatting
    
    PreCommit->>Isort: Sort imports
    Isort->>Code: Reorder imports
    
    Code-->>Dev: Code with mixed linting/formatting
    
    Note over Code: Issues introduced:<br/>1. Logic changes in importer.py<br/>2. Breaking change in spaces.py<br/>3. Comparison reversals<br/>4. Python version mismatch
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

6 files reviewed, 6 comments

Edit Code Review Agent Settings | Greptile

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 13, 2026

Additional Comments (1)

pyproject.toml
The Black and isort configurations are still present in pyproject.toml. If the migration to Ruff is complete, these sections should be removed to avoid confusion.

The comment on line 12 says "TODO: Remove this once isort is removed from the project", but this contradicts the PR description which states the linting setup has been replaced with Ruff.

Based on the .pre-commit-config.yaml, it appears Black and isort are still in use, so this PR is only a partial migration to Ruff (linting only, not formatting/import sorting).

@Mayankm96 Mayankm96 merged commit 6cb2a7e into isaac-sim:main Jan 13, 2026
6 checks passed
@github-project-automation github-project-automation bot moved this from In progress to Done in Isaac Lab Jan 13, 2026
@Mayankm96 Mayankm96 deleted the feature/ruff branch January 13, 2026 15:48
Edify0991 pushed a commit to Edify0991/IsaacLab that referenced this pull request Jan 14, 2026
# Description

Ruff can handle linting, formatting, and type-checking (where
applicable), streamlining our development workflow and improving code
quality.

This PR replaces our current linting setup with Ruff. Subsequent MRs
will look into using Ruff for formatting and import ordering.

## Type of change

- Documentation update

## Checklist

- [x] I have read and understood the [contribution
guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html)
- [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with
`./isaaclab.sh --format`
- [x] I have made corresponding changes to the documentation
- [ ] My changes generate no new warnings
- [ ] I have added tests that prove my fix is effective or that my
feature works
- [ ] I have updated the changelog and the corresponding version in the
extension's `config/extension.toml` file
- [x] I have added my name to the `CONTRIBUTORS.md` or my name already
exists there

---------

Signed-off-by: Mayank Mittal <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

asset New asset feature or request documentation Improvements or additions to documentation enhancement New feature or request infrastructure isaac-lab Related to Isaac Lab team isaac-mimic Related to Isaac Mimic team

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant