Skip to content

Conversation

@samarthnaikk
Copy link

Overview: What does this pull request change?

This pull request refactors the Torus class in manim/mobject/three_d/three_dimensions.py by renaming the field self.r to self.minor_radius. This change improves code clarity and reduces potential confusion with the self.R (major_radius) field. All internal usages of the field have been updated accordingly.

  • Refactored Torus class by renaming the ambiguous self.r attribute to self.minor_radius.

Motivation and Explanation: Why and how do your changes improve the library?

The motivation for this change is to enhance code quality and maintainability.

  • Clarity: The field self.r was used to represent the minor radius of the torus, while self.R represents the major radius. The minimal visual difference between r and R can easily lead to misinterpretation, bugs, and confusion, especially for new contributors.
  • Improved Maintainability: Renaming self.r to the more descriptive self.minor_radius makes the purpose of the field immediately clear, which simplifies future maintenance and development.
  • Tool Conformance: This change resolves a "code smell" flagged by static analysis tools like SonarQube regarding unclear variable names.

The change was implemented by renaming the attribute and using refactoring to update all its occurrences within the Torus class, ensuring functionality remains identical.

Links to added or changed documentation pages

This change primarily affects the internal implementation of the Torus class. The docstrings within manim/mobject/three_d/three_dimensions.py have been reviewed to ensure they reflect the new attribute name. No public-facing documentation pages were directly affected.

Further Information and Comments

The field self.r in the Torus class has been renamed to self.minor_radius, and all usages within the class have been updated. The implementation was verified against related test files, and no regressions were found. The code is now more maintainable and aligns with best practices for clear naming conventions.

Reviewer Checklist

  • The PR title is descriptive enough for the changelog, and the PR is labeled correctly
  • If applicable: newly added non-private functions and classes have a docstring including a short summary and a PARAMETERS section
  • If applicable: newly added functions and classes are tested

this PR should solve #4271

@samarthnaikk
Copy link
Author

Are there any changes that are still expected ? @henrikmidtiby

@chopan050
Copy link
Contributor

Thanks for your contribution!

Unfortunately, the original issue #4271 which this PR fixes has been deemed as AI slop, along with issues #4272 and #4273 from the same author which are even worse. All three issues miss some key context.

Quoting the comment I made there:

Although this issue does raise a point regarding making the attributes more descriptive, it fails to notice that, if it suggests renaming self.r to self.minor_radius, then it should have also suggested renaming self.R to self.major_radius. Nevertheless, after some discussion, it is not planned to make this change. Those attributes aren't really used outside of the class definition, there is a sufficient visual difference between r and R, and their meanings as minor and major radius can be inferred from the context (radii of a torus).

Sorry for that!

@chopan050 chopan050 closed this Nov 20, 2025
@github-project-automation github-project-automation bot moved this from 🆕 New to ❌ Rejected in Dev Board Nov 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Rejected

Development

Successfully merging this pull request may close these issues.

2 participants