Replace axis_artist.AttributeCopier by normal inheritance. #13452
Conversation
There was a problem hiding this comment.
Is this whole ref_artist business still a thing then? If not get_ref_artist should also be deprecated in the derived classes and one should call the respective artist directly.
If so, I think it's reasonable to have some sort of pattern for it. Not necessarily full inheritance, but maybe a mixin, a decorator or function generator (Not thought this through - some of these may not be suitable).
That doesn't work because TickLabels relies on just overriding get_ref_artist and then using its parent class' get_color.
I had a quick try and it's a bit of a pain (especially for a one-off). Patches welcome, though. |
| @@ -211,14 +224,17 @@ def __init__(self, ticksize, tick_out=False, *, axis=None, **kwargs): | ||
| kwargs["markeredgewidth"] = "auto" | ||
|
|
||
| Line2D.__init__(self, [0.], [0.], **kwargs) | ||
| AttributeCopier.__init__(self, self._axis, klass=Line2D) | ||
| self._klass = Line2D # Only during the AttributeCopier deprecation. | ||
| self.set_snap(True) | ||
|
|
||
| def get_ref_artist(self): |
There was a problem hiding this comment.
Can we make this private (_get_ref_artist)? I don't think ref artists need to be part of the public API.
Also please add a docstring such as:
Return an underlying artist, that actually defines the properties (e.g. color) of self.
There was a problem hiding this comment.
Honestly I wouldn't bother with the deprecation; it's not as if there's any plan to actually change that method.
Added the docstring.
There was a problem hiding this comment.
You've deprecated the whole AttributeCopier, which originally introduced get_ref_artist. It would only be consquent to remove get_ref_artist as well from the public API. This is foremost for cleaning up the public API. ref artists are an implementation detail that should not be publicly visible. Users should not be bothered with this (neither in the docs nor in parameter completion etc.). Future ability to change is only the weaker motivation.
b93c81b to
810b092
Compare
|
@timhoffm I figured out how to make the ref_artist delegation pattern work, so now it lives in AttributeCopier and the deprecation is much more limited. |
|
Nice! That's a good pattern. 👍 The additional question is, do we need it as public API? If not I would still deprecate the whole |
810b092 to
46a5d94
Compare
|
Personally I find it a bit strange for a public class to inherit from a private class (for example, the actual docstring of get_ref_artist (a public API) would live in a private class. |
Note that the "default" provided to get_attribute_from_ref_artist would previously never be triggered, as there is always a ref_artist present.
|
rebased |
Note that the "default" provided to get_attribute_from_ref_artist would
previously never be triggered, as there is always a ref_artist present.
PR Summary
PR Checklist