Mark some methods as internal #3793

JasonGrace2282 · 2024-05-30T17:29:40Z

This is an idea of an implementation for #3792

It defines a decorator that modifies the function's docstring to warn end-users that it is marked as an implementation detail.

Example

TODO

Figure out why sometimes the decorator does not produce the warning directive

manim/utils/decorators.py

behackl

This looks fine! Left one suggestion and a bunch of comments where I thought it is somewhat debatable that a method would be marked as internal.

I like the mechanism though, am happy to merge this in principle. Thanks for your work!

behackl · 2024-07-13T17:27:03Z

manim/mobject/mobject.py

@@ -1712,6 +1714,7 @@ def stretch_to_fit_depth(self, depth: float, **kwargs) -> Self:

        return self.rescale_to_fit(depth, 2, stretch=True, **kwargs)

+    @internal


To be honest, I don't remember. However, I think that it's more common to use move_to or set_(x|y|z) as opposed to this method.
This PR was mostly an introduction of concept, so I'm happy to remove it if you disagree.

behackl · 2024-07-13T17:27:12Z

manim/mobject/mobject.py

@@ -1902,6 +1905,7 @@ def set_colors_by_radial_gradient(
        )
        return self

+    @internal


There was already a set_colors_by_gradient that used this internally and returning Self, so I figured that should be the public one.

behackl · 2024-07-13T17:27:18Z

manim/mobject/mobject.py

@@ -1915,6 +1919,7 @@ def set_submobject_colors_by_gradient(self, *colors: Iterable[ParsableManimColor
            mob.set_color(color, family=False)
        return self

+    @internal


Here, there was already a set_colors_by_radial_gradient that used this internally and returning Self, so I figured that should be the public one.

behackl · 2024-07-13T17:35:24Z

manim/mobject/mobject.py

@@ -2044,6 +2049,7 @@ def get_points_defining_boundary(self) -> Point3D_Array:
    def get_num_points(self) -> int:
        return len(self.points)

+    @internal


maybe also borderline

manim/mobject/mobject.py

behackl · 2024-07-13T17:36:39Z

manim/mobject/mobject.py

@@ -2784,6 +2795,7 @@ def add_n_more_submobjects(self, n: int) -> Self | None:
    def repeat_submobject(self, submob: Mobject) -> Self:
        return submob.copy()

+    @internal


borderline?

behackl · 2024-07-13T17:36:53Z

manim/mobject/types/vectorized_mobject.py

@@ -708,6 +712,7 @@ def set_shade_in_3d(
                submob.z_index_group = self
        return self

+    @internal


I mostly added this because of discussions on whether to make points a property (to add some memoization stuff).

behackl · 2024-07-13T17:39:40Z

manim/mobject/types/vectorized_mobject.py

@@ -1800,6 +1834,7 @@ def get_point_mobject(self, center: Point3D | None = None) -> VectorizedPoint:
        point.match_style(self)
        return point

+    @internal


borderline?

behackl · 2024-07-13T17:39:47Z

manim/mobject/types/vectorized_mobject.py

@@ -1824,6 +1859,7 @@ def interpolate_color(
                    val = val.copy()
                setattr(self, attr, val)

+    @internal


borderline?

I find it hard to think of a case where a user would need this, and (I can't remember) but there's a chance this changes in experimental.

manim/utils/decorators.py

Co-authored-by: Benjamin Hackl <[email protected]>

JasonGrace2282 · 2024-07-14T19:49:19Z

Before this PR is merged, it might also be worth checking if there is a performance penalty.
Currently, this implementation does not take docstrings from parent classes (which is what inspect.getdoc does), as doing so is obviously slower - maybe there's a way we can only modify the docstrings while sphinx is building the docs?
EDIT: Environment variables might be the best way to go here - according to the os docs environment variables are stored in memory so it should just be a lookup away?

EDIT 2: Just did some benchmarking, the performance penalty from inspect.cleandoc is very low. Considering that it is only applied once at import time, it should be negligible.

JasonGrace2282 · 2024-07-14T19:57:21Z

it is somewhat debatable that a method would be marked as internal.

My view is always that when in doubt, mark it as internal. This gives us the most flexibility for the future (even though we're not at 1.0.0 yet!), and changing something from internal → public is not a major semvar change.

JasonGrace2282 added 2 commits May 30, 2024 13:30

Add decorator

8d7bb13

Mark some methods as internal on Mobject/VMobject

bfea7da

JasonGrace2282 force-pushed the internal branch from 09ee2df to bfea7da Compare May 30, 2024 17:30

JasonGrace2282 marked this pull request as draft May 30, 2024 17:48

Fix docstrings being weird and causing build warnings

bd7eae7

JasonGrace2282 marked this pull request as ready for review May 31, 2024 01:57

Viicos reviewed May 31, 2024

View reviewed changes

manim/utils/decorators.py Outdated Show resolved Hide resolved

JasonGrace2282 and others added 4 commits May 31, 2024 08:00

don't use ParamSpec

d5377cb

Merge branch 'main' into internal

b0df579

Change spacing

97b7a57

Merge branch 'main' into internal

0eddd98

JasonGrace2282 added this to the v0.19.0 milestone Jun 10, 2024

behackl reviewed Jul 13, 2024

View reviewed changes

JasonGrace2282 and others added 2 commits July 14, 2024 09:57

Improve decorator message

10fa509

Co-authored-by: Benjamin Hackl <[email protected]>

Some feedback

c6c6533

Merge branch 'main' into internal

82d39f2

JasonGrace2282 added enhancement Additions and improvements in general needs discussion Things which needs to be discussed before implemented. labels Jul 25, 2024

JasonGrace2282 modified the milestones: v0.19.0, v0.20.0 Jul 25, 2024

JasonGrace2282 removed the needs discussion Things which needs to be discussed before implemented. label Aug 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mark some methods as internal #3793

Mark some methods as internal #3793

JasonGrace2282 commented May 30, 2024 •

edited

Loading

behackl left a comment

behackl Jul 13, 2024

JasonGrace2282 Jul 14, 2024

behackl Jul 13, 2024

JasonGrace2282 Jul 14, 2024

behackl Jul 13, 2024

JasonGrace2282 Jul 14, 2024

behackl Jul 13, 2024

behackl Jul 13, 2024

behackl Jul 13, 2024

JasonGrace2282 Jul 14, 2024

behackl Jul 13, 2024

behackl Jul 13, 2024

JasonGrace2282 Jul 14, 2024

JasonGrace2282 commented Jul 14, 2024 •

edited

Loading

JasonGrace2282 commented Jul 14, 2024 •

edited

Loading

		@@ -1712,6 +1714,7 @@ def stretch_to_fit_depth(self, depth: float, **kwargs) -> Self:

		return self.rescale_to_fit(depth, 2, stretch=True, **kwargs)

		@internal

Mark some methods as internal #3793

Are you sure you want to change the base?

Mark some methods as internal #3793

Conversation

JasonGrace2282 commented May 30, 2024 • edited Loading

TODO

behackl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonGrace2282 commented Jul 14, 2024 • edited Loading

JasonGrace2282 commented Jul 14, 2024 • edited Loading

JasonGrace2282 commented May 30, 2024 •

edited

Loading

JasonGrace2282 commented Jul 14, 2024 •

edited

Loading

JasonGrace2282 commented Jul 14, 2024 •

edited

Loading