Introduce ArtistList for FigureBase#31746
Conversation
|
It seems I have discovered something that only works at py314 🧐 |
This comment was marked as outdated.
This comment was marked as outdated.
527f6fe to
81d29da
Compare
Following matplotlib#18216 for Axes artists, combine all figure artists except axes and subfigures into a single list and deprecate modifying the lists directly.
| if sys.version_info[:2] < (3, 14): | ||
| # Including the collections.abc string in skip_file_prefixes is not yet honored. | ||
| # Add the relevant frame count to the stacklevel instead. |
There was a problem hiding this comment.
I didn't find anything in the python changenotes to understand what changed here, but I have verified that the relevant test passes with python 3.14.0.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
The inheritance diagram in the artists API docs broke without these.
This comment was marked as outdated.
This comment was marked as outdated.
| A sublist of Figure children based on their type. This subclass exists only to | ||
| provide deprecation warnings. When the deprecations expire, use ArtistList |
There was a problem hiding this comment.
Wouldn't it be simpler to add
if isinstance(parent, FigureBase):
_api.warn_deprecated(...)or even
self.warn_deprecated_if_on_figure(...)to the regular ArtistList methods?
There was a problem hiding this comment.
ArtistList is already immutable. It doesn’t have these methods.
There was a problem hiding this comment.
Ok, I see. There's a lot of unforeseen complexity in the migration of ArtistList from AxesBase to artist, which gives me a bit of a headeache overlooking the complete PR. In hindsight it would likely have been better to either split this in two PRs / commits: (1) move ArtistList (2) use ArtistList in Figure; or alternatively just duplicate the 70 lines of ArtistList in Figure, and when the Figure deprecations run out and both classes become identical merge them back together. But I'm somewhat reluctant to request that now.
There was a problem hiding this comment.
Ah sorry. It did occur to me when I was moving it that splitting into separate commits for moving and modifying might help the review, but at the time I couldn't quite see how to split it. I will have a go at just moving it in a separate branch and if it works I'll make a prequel PR.
|
Hid the discussion about doc-failure debugging as that is now fixed in #31794 and summarised at #31794 (comment). |
PR summary
At #31730 (comment) Tom said
This PR generalises the
ArtistListso it can work for both figures and axes. The new_FigureArtistListsubclasses it just to add the deprecated methods. I adapted the deprecated methods and their tests from #18216, where this was originally done forAxes. Note thatfig.subfiguresis still a plain list. I do not think subfigures belong infig._children, but rather should have their own handling similar tofig.axes. That can be done separately.AI Disclosure
I asked duck.ai how to get a class as a string, to use in the
ArtistListreprself._parent.__class__.__name__.PR checklist