Cleanup initialization in text()#12215
Conversation
f478d0f to
5015ae9
Compare
5015ae9 to
848fa10
Compare
|
In this specific case, I'd just deprecate TextWithDash in favor of Annotation (possibly after checking that the latter indeed replaces (reasonable...) use cases of the former). |
|
Even the scaled back version with a single update does yield different plots with python 3.5 on travis. There's something fishy going on if three sequential updates have a different effect compared to one combined update. Marking as WIP because this needs further investigation. |
|
Ah, dict iteration order is random in 3.5. This fix may have to wait until we drop 3.5 (which I suspect will be for the next release, but someone needs to turn the crank on making that decision). |
|
Thanks, that's it. Looking forward to drop 3.5, then we can also use f-strings 😄. |
848fa10 to
8d31731
Compare
|
Rebased. Should hopefully work now that were're on Python 3.6+. Can be squashed. |
|
want to squash? |
8d31731 to
54ada60
Compare
|
Squashed. |
|
So this is still not going to do the full constructor change even on 3.6+? |
|
@QuLogic Whst do you mean by „full constructor change“? This is a basic simplification to just do a single update instead of updating multiple times. |
|
I mean the original description:
|
|
Oh, that 😄 (too long ago to remember). Because of
I've restricted myself to
The call sequence issue is not solved with python 3.6+. Also @anntzer proposed to deprecate TL;DRNo, this won't do the full constructor change. |
|
PR title and original description updated. |
PR Summary
Axes.text()created a defaultTextand then calledText.update()three times to adapt to the passed kwargs. This is a bit inefficient and cumbersome.This PR changes the code to create effective kwargs and
directly pass them to theonly update once.Text()constructor.This should not have any effect except being a little faster.I also had to fixTextWithDashto accept all the kwargs ofTextin the constructor. This is a bugfix asTextWithDashclaims to be a drop-in replacement forText.Turns out, making
TextWithDashfully API compatible withTextis a bit more cumbersome. Just passing the kwargs on is not enough. There is an issue with the call sequenceTextWithDashes.__init__->Text.__init__->TextWithDashes.set_transform. I.e. the super init calling a child function before all the attributes of the child are set up.Options are:
super().__init__()is the last command inTextWithDashes.__init__instead of the first. However, that seems just like a workaround as well. See Init calls subclass method pattern #12220 for further discussion.For now, I've resorted to the defensive option 1.