-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Cleanup initialization in text() #12215
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modulo ci
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 defaultText
and 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 fixTextWithDash
to accept all the kwargs ofText
in the constructor. This is a bugfix asTextWithDash
claims to be a drop-in replacement forText
.Turns out, making
TextWithDash
fully API compatible withText
is 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.