add typos to pre-commit#982
Conversation
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
📝 WalkthroughWalkthroughAdds typo-detection tooling (pre-commit hook and pyproject.toml settings), inserts Typos usage documentation into the development guide, and renames a loop variable in a MuJoCo solver method; no public API or functional behavior changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
2370-2398: Avoid shadowing the built-intypefunction.Renaming the loop variable from
typtotypeshadows Python's built-intype()function, which violates PEP 8 guidelines and can lead to confusion. While this doesn't cause functional issues in this specific scope, it's an anti-pattern that should be avoided.Consider one of these alternatives:
Option 1 (Recommended): Revert to
typand add it as an exception in the typos configuration:- for i, type in enumerate(eq_constraint_type): - if type == EqType.CONNECT: + for i, typ in enumerate(eq_constraint_type): + if typ == EqType.CONNECT: eq = spec.add_equality(objtype=mujoco.mjtObj.mjOBJ_BODY) eq.type = mujoco.mjtEq.mjEQ_CONNECT eq.active = eq_constraint_enabled[i] eq.name1 = model.body_key[eq_constraint_body1[i]] eq.name2 = model.body_key[eq_constraint_body2[i]] eq.data[0:3] = eq_constraint_anchor[i] - elif type == EqType.JOINT: + elif typ == EqType.JOINT: eq = spec.add_equality(objtype=mujoco.mjtObj.mjOBJ_JOINT) eq.type = mujoco.mjtEq.mjEQ_JOINT eq.active = eq_constraint_enabled[i] eq.name1 = model.joint_key[eq_constraint_joint1[i]] eq.name2 = model.joint_key[eq_constraint_joint2[i]] eq.data[0:5] = eq_constraint_polycoef[i] - elif type == EqType.WELD: + elif typ == EqType.WELD: eq = spec.add_equality(objtype=mujoco.mjtObj.mjOBJ_BODY) eq.type = mujoco.mjtEq.mjEQ_WELDThen add to
pyproject.toml:[tool.typos.default.extend-identifiers] typ = "typ"Option 2: Use a more descriptive name:
- for i, type in enumerate(eq_constraint_type): - if type == EqType.CONNECT: + for i, constraint_type in enumerate(eq_constraint_type): + if constraint_type == EqType.CONNECT:Note: This pattern also appears elsewhere in the file (lines 351, 427, 636, 985). Consider addressing those instances as well for consistency.
🧹 Nitpick comments (2)
pyproject.toml (1)
203-211: Typos configuration looks good, but consider addingtypexception.The configuration appropriately excludes asset files and format-specific files from typo checking. The word exceptions are reasonable.
Consider adding
typto the identifiers list to prevent false positives, as it's commonly used as an abbreviation for "type" (especially to avoid shadowing Python's built-in):[tool.typos.default.extend-identifiers] PNGs = "PNGs" +typ = "typ"This would allow using
typas a variable name without triggering typo warnings, which is preferable to shadowing the built-intypefunction..pre-commit-config.yaml (1)
29-32: Consider updating typos to v1.38.1.The latest stable version is v1.38.1 (released Oct 7, 2025), whereas v1.37.2 is currently specified. While the current version works, updating to the latest stable release is a good practice for staying current with improvements and fixes.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.pre-commit-config.yaml(1 hunks)newton/_src/solvers/mujoco/solver_mujoco.py(1 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
newton/_src/sim/joints.py (1)
EqType(99-114)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
- GitHub Check: Run GPU Benchmarks (Pull Request)
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
|
Seems fine to me, but I wonder how easy is it for someone else to add a false positive to Another option is to add a GitHub check for |
|
How are you managing this with warp? I'm open to both approaches. Maybe we should move the configt to a separate dotfile such that we don't convolute pyproject.toml. |
Yeah, that might help users edit the ignore lists, Personally, I like all the tool configs being in For Warp, I just make a |
|
let me add some docs to the development documentation. I think it's easier to require typos to pass instead of manually chasing things down after the fact. |
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/guide/development.rst (2)
164-168: Clear introduction to the typos tool.The section effectively explains the purpose and integration of typos with pre-commit.
Consider adding a brief example of the manual command for completeness:
To proactively catch spelling mistakes, Newton uses the `typos <https://github.com/crate-ci/typos>`_ tool. Typos scans source files for common misspellings and is integrated into our pre-commit hooks, so spelling errors in both code and documentation are flagged when you run or install pre-commit (see above). You can also run ``typos`` manually if needed. Refer to the `typos documentation <https://github.com/crate-ci/typos>`_ for more details on usage and configuration options. + +To run typos manually: + +.. code-block:: console + + typos
169-181: Comprehensive guide for handling false positives.The subsection clearly explains the three configuration mechanisms and provides helpful guidance on updating
pyproject.tomlto suppress warnings.Minor: Lines 182-185 contain excessive blank lines. Consider reducing to a single blank line for consistency with the rest of the document:
When typos reports a word that is valid within the Newton codebase, you can add it to the appropriate section in ``pyproject.toml`` to suppress future warnings. After updating, re-run typos (or pre-commit) to confirm that the word is ignored. Use these options to keep the codebase clean while ensuring needed flexibility for accepted project-specific words and identifiers. - - - - + Using a local Warp installation with uv
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/guide/development.rst(1 hunks)newton/_src/solvers/mujoco/solver_mujoco.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
newton/_src/sim/joints.py (1)
EqType(99-114)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run GPU Benchmarks (Pull Request)
🔇 Additional comments (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
2370-2398: LGTM! Variable rename improves readability.The refactor from
typtoconstraint_typemakes the code more self-documenting and aligns well with the iteration overeq_constraint_type. All three conditional branches are updated consistently.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/guide/development.rst (1)
164-181: Well-structured documentation for typos integration and false-positive handling.The new Typos section clearly explains the tool's purpose, pre-commit integration, and provides actionable guidance for managing false positives through file exclusions, word allowlists, and identifier allowlists. The examples (ba, HAA, PNGs, *.urdf) are concrete and contextually appropriate for a physics simulation library.
The placement after pre-commit install instructions is logical, and the documentation aligns well with the PR's goal of adding this information per reviewer feedback.
Optional improvement: Line 167 mentions running typos "manually if needed" but doesn't show the command. For completeness, consider adding a brief code example showing how to invoke typos directly (e.g., via
uvx typosortypos .), similar to how the pre-commit section shows concrete commands above. This would help users who want to run the check outside the pre-commit workflow without consulting external documentation.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/guide/development.rst(1 hunks)newton/_src/solvers/mujoco/solver_mujoco.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- newton/_src/solvers/mujoco/solver_mujoco.py
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
add typos to pre-commit.
Not sure if we really need this given the fact that there were not a lot of things to fix.. but let's have a discussion.
Summary by CodeRabbit