Skip to content

add typos to pre-commit#982

Merged
adenzler-nvidia merged 7 commits into
newton-physics:mainfrom
adenzler-nvidia:dev/adenzler/typos
Oct 27, 2025
Merged

add typos to pre-commit#982
adenzler-nvidia merged 7 commits into
newton-physics:mainfrom
adenzler-nvidia:dev/adenzler/typos

Conversation

@adenzler-nvidia

@adenzler-nvidia adenzler-nvidia commented Oct 24, 2025

Copy link
Copy Markdown
Member

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

  • Chores
    • Added automated typo detection to pre-commit and project configuration to surface and manage spelling/identifier issues.
  • Documentation
    • Added guidance to the development guide on running the typo checker, configuring suppressions, exclusions, and allowlists.
  • Refactor
    • Minor internal code cleanup to improve readability without changing behavior.

Signed-off-by: Alain Denzler <adenzler@nvidia.com>
@adenzler-nvidia adenzler-nvidia marked this pull request as draft October 24, 2025 09:22
@coderabbitai

coderabbitai Bot commented Oct 24, 2025

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

Adds 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

Cohort / File(s) Summary
Typos tooling
/.pre-commit-config.yaml, pyproject.toml
Added pre-commit hook entry for crate-ci/typos (rev: v1.38.1, id: typos) and [tool.typos] configuration in pyproject.toml with file exclude patterns and extended words/identifiers.
Documentation
docs/guide/development.rst
Inserted a new "Typos" section (duplicated in two locations) describing the pre-commit integration, how to run Typos, handling false positives, file exclusions, and allowlists.
MuJoCo solver refactor
newton/_src/solvers/mujoco/solver_mujoco.py
Renamed loop variable from typ to constraint_type in add_geoms and updated comparisons to use constraint_type; no behavioral or signature changes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Review the variable rename in newton/_src/solvers/mujoco/solver_mujoco.py to ensure no remaining references to the old name and no semantic drift.
  • Check the duplicated insertion in docs/guide/development.rst for intended duplication or if one instance should be removed.
  • Validate typos exclusions and allowlists in pyproject.toml match repository structure and desired suppressions.

Suggested reviewers

  • eric-heiden

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "add typos to pre-commit" directly and specifically describes the primary objective of the changeset. The author is integrating the typos tool into the repository's pre-commit workflow, which is reflected across multiple files: .pre-commit-config.yaml (hook configuration), pyproject.toml (tool configuration), newton/_src/solvers/mujoco/solver_mujoco.py (a variable rename likely resolving a typo issue), and docs/guide/development.rst (documentation). The title is concise, clear, and sufficiently specific that a teammate reviewing the commit history would immediately understand the primary change without ambiguity.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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-in type function.

Renaming the loop variable from typ to type shadows Python's built-in type() 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 typ and 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_WELD

Then 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 adding typ exception.

The configuration appropriately excludes asset files and format-specific files from typo checking. The word exceptions are reasonable.

Consider adding typ to 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 typ as a variable name without triggering typo warnings, which is preferable to shadowing the built-in type function.

.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

📥 Commits

Reviewing files that changed from the base of the PR and between acf8c90 and e4efb34.

📒 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>
Comment thread pyproject.toml Outdated
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
@shi-eric

Copy link
Copy Markdown
Member

Seems fine to me, but I wonder how easy is it for someone else to add a false positive to pyproject.toml? Maybe we can add something to the docs that explains.

Another option is to add a GitHub check for typos that is allowed to fail so false positives don't block any pull requests from being merged, and we can add the exclusions to pyproject.toml on our own cadence.

@adenzler-nvidia

Copy link
Copy Markdown
Member Author

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.

@shi-eric

Copy link
Copy Markdown
Member

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 pyproject.toml.

For Warp, I just make a typos pass required. I haven't seen any issues with doing that so far, but it's also been around for a few years so there was a lot of material to build the ignore lists off of when I set typos up.

@adenzler-nvidia

Copy link
Copy Markdown
Member Author

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>
@adenzler-nvidia adenzler-nvidia marked this pull request as ready for review October 27, 2025 15:46
Signed-off-by: Alain Denzler <adenzler@nvidia.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.toml to 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

📥 Commits

Reviewing files that changed from the base of the PR and between dd279ca and 2180c1d.

📒 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 typ to constraint_type makes the code more self-documenting and aligns well with the iteration over eq_constraint_type. All three conditional branches are updated consistently.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 typos or typos .), 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

📥 Commits

Reviewing files that changed from the base of the PR and between dd279ca and 2180c1d.

📒 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

@adenzler-nvidia adenzler-nvidia merged commit 56c25d1 into newton-physics:main Oct 27, 2025
18 checks passed
eric-heiden pushed a commit to eric-heiden/newton that referenced this pull request Jan 28, 2026
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
mmacklin pushed a commit to mmacklin/newton that referenced this pull request Apr 7, 2026
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants