Skip to content

Conversation

christopher-hampson
Copy link
Contributor

Overview: What does this pull request change?

Adds set_default class method to Animation class based on existing implementation for Mobject class.

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

Addresses issue #3142

Links to added or changed documentation pages

Further Information and Comments

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

Adds set_default class method to Animation class based on existing implementation for Mobject class.

Addresses issue ManimCommunity#3142
@JasonGrace2282
Copy link
Member

JasonGrace2282 commented Jul 20, 2024

Hello, thanks for sending a PR for this feature :)
Off of a quick look, the implementation seems fine (I'll take a closer look, hopefully later this week). Would you mind adding some tests for this behavior?
Thanks for helping make Manim better ✨

P.S. Don't worry about the pre-commit failure. We'll fix that in a different PR.

Copy link
Member

@JasonGrace2282 JasonGrace2282 left a comment

Choose a reason for hiding this comment

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

Just tested and it works for me! A simple test to add would be something like

def test_animation_set_default():
    s = Square()
    Rotate.set_default(run_time=100)
    anim = Rotate(s)
    assert anim.run_time == 100

Other than that, looks good to me :)

@christopher-hampson
Copy link
Contributor Author

Just tested and it works for me! A simple test to add would be something like

def test_animation_set_default():
    s = Square()
    Rotate.set_default(run_time=100)
    anim = Rotate(s)
    assert anim.run_time == 100

Other than that, looks good to me :)

Thanks for reviewing and suggesting this test! I'll get this added shortly. 👍

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.

Just some minor docstring formatting suggestions; I'll just apply these whitespace changes.

Thanks for your contribution!

@behackl behackl requested a review from JasonGrace2282 July 22, 2024 09:39
Copy link
Member

@JasonGrace2282 JasonGrace2282 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks :)

@behackl behackl added the new feature Enhancement specifically adding a new feature (feature request should be used for issues instead) label Jul 22, 2024
@behackl behackl added this to the v0.19.0 milestone Jul 22, 2024
@behackl behackl merged commit 0843632 into ManimCommunity:main Jul 22, 2024
@JasonGrace2282 JasonGrace2282 linked an issue Jul 23, 2024 that may be closed by this pull request
@christopher-hampson christopher-hampson deleted the feature/animation-set-default branch September 3, 2024 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

new feature Enhancement specifically adding a new feature (feature request should be used for issues instead)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Animation.set_default

3 participants