Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
f478d0f
to5015ae9
CompareIn 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 😄. |
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? |
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. |
Uh oh!
There was an error while loading.Please reload this page.
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. SeeInit calls subclass method pattern #12220 for further discussion.For now, I've resorted to the defensive option 1.