Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Doc: add docstrings to Brace #3715

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Doc: add docstrings to Brace #3715

wants to merge 8 commits into from

Conversation

biinnnggggg
Copy link
Contributor

Overview: What does this pull request change?

Add docstrings to methods in Brace so that sphinx will create hyperlinks to these methods.

Links to added or changed documentation pages

Further Information and Comments

Related to #3597

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

@JasonGrace2282
Copy link
Contributor

JasonGrace2282 commented Apr 22, 2024 •
edited

Is it possible to add some doc tests as well?
You can see an example here.
Thanks for improving the documentation :)

@JasonGrace2282 JasonGrace2282 added the documentation Improvements or additions to documentation label Apr 22, 2024
behackl
behackl requested changes Apr 24, 2024
View reviewed changes
Copy link
Member

@behackl behackl left a comment

Choose a reason for hiding this comment

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

Looks fine, thanks! I've left one minor comment about the added type hint, please take a look.

manim/mobject/svg/brace.py Outdated Show resolved Hide resolved
JasonGrace2282
JasonGrace2282 approved these changes May 28, 2024
View reviewed changes
Copy link
Contributor

@JasonGrace2282 JasonGrace2282 left a comment •
edited

Choose a reason for hiding this comment

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

LGTM! Thanks for your work here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JasonGrace2282 JasonGrace2282 JasonGrace2282 approved these changes

@behackl behackl Awaiting requested review from behackl

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
documentation Improvements or additions to documentation
Projects
Dev Board
Status: 👀 In review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@biinnnggggg @JasonGrace2282 @behackl