NotificationsYou must be signed in to change notification settings
Fork7.9k
Star21.4k

Soft deprecate the textpath module (import from text instead)#23576

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

Jump to bottom

Merged

QuLogic merged 1 commit intomatplotlib:mainfromoscargus:softdeprecatetextpath

Dec 21, 2022

Merged

Soft deprecate the textpath module (import from text instead)#23576

QuLogic merged 1 commit intomatplotlib:mainfromoscargus:softdeprecatetextpath

Dec 21, 2022

Conversation

Copy link

Member

oscargus commentedAug 7, 2022

The textpath module was created in 2009, but the status has
been a bit vague with many examples and exisiting code found
on the internet importing from text instead.

In this PR everything is changed to point at text, although textpath
is still available for backwards compatibility.

PR Summary

See discussion in#23565 (comment)

Also a bit of cleanup of related documentation.

PR Checklist

Tests and Styling

[N/A] Has pytest style unit tests (andpytest passes).
IsFlake 8 compliant (installflake8-docstrings and runflake8 --docstring-convention=all).

Documentation

[N/A] New features are documented, with examples if plot related.
[N/A] New features have an entry indoc/users/next_whats_new/ (follow instructions in README.rst there).
[N/A] API changes documented indoc/api/next_api_changes/ (follow instructions in README.rst there).
[N/A] Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).

oscargus added Documentation topic: text API: consistency labels

Aug 7, 2022

oscargus mentioned this pull request

Aug 7, 2022

Deprecate incorrect import locations#23565

Closed

2 tasks

Copy link

Member

tacaswell commentedAug 17, 2022

I'm 50/50 on this. If anything I think the error is the places than are importing transitively throughmatplotilb.text not the other way around....

Copy link

MemberAuthor

oscargus commentedAug 18, 2022

There is also#23565 where the consensus seems to be that this is the correct solution. Somehow, one of them should be...

github-actionsbot added the status: needs rebase label

Oct 18, 2022

tacaswell added this to thev3.7.0 milestone

Dec 16, 2022

Copy link

Member

tacaswell commentedDec 16, 2022

Re-reading the conversation here and in#23565 , the consensus seems to be in favor of this path (I'm still a coin flip), so lets go with this for 3.7.

Copy link

Member

ksunden commentedDec 16, 2022

Should we add a__all__ totext.py to make it even more explicit thatTextPath is an exported public api?

The potential downside of course being that things we don't consider "public" would no longer be imported if someone didfrom matplotlib.text import * ...

That said, this mostly applies to transitive imports since anything defined here would be exported and_methods are explicitly private. so this would only really affect people doing things like usingArtist ornp from the* import oftext, which is not intended usage)

The other one that is closely related enough that I'd consider it reasonable to think "that is in text" would beFontProperties, Everything else is pretty clearly not "text" and so should not be expected to be imported viampl.text I would expect.

timhoffm reviewed

Dec 17, 2022

View reviewed changes

lib/matplotlib/backends/backend_svg.pyShow resolvedHide resolved

Copy link

Member

timhoffm commentedDec 17, 2022

The overall strategiy is good.

Should we add a__all__ totext.py to make it even more explicit thatTextPath is an exported public api?

Unfortunately. specification of public API does not have proper language support in python.__all__ is only a band aid and not really suited for the task. I would refrain from this.

Another issue is thatTextPath will not disguise its origin. The fully qualified name will staymatplotlib.textpath.TextPath however much we document and__all__. Well, we could monkey patch that, but that's not a good idea either. It may be reasonable to bite the bullet and actually move the code totext. Then, reverse the import andfrom matplotlib.text import TextPath intextpath for backward compatibility. But that can be done later.

oscargus force-pushed thesoftdeprecatetextpath branch fromaa0bf63 toc00711bCompare

December 17, 2022 10:39

github-actionsbot removed the status: needs rebase label

Dec 17, 2022

oscargus force-pushed thesoftdeprecatetextpath branch fromc00711b to71a3685Compare

December 17, 2022 10:44

Soft deprecate the textpath module (import from text instead)

9b8a598

The textpath module was created in 2009, but the status hasbeen a bit vague with many examples and exisiting code foundon the internet importing from text instead.In this PR everything is changed to point at text, although textpathis still available for backwards compatibility.

oscargus force-pushed thesoftdeprecatetextpath branch from71a3685 to9b8a598Compare

December 17, 2022 10:46

Copy link

MemberAuthor

oscargus commentedDec 17, 2022

Another issue is thatTextPath will not disguise its origin.

As long as we use and document it consistently it will be better than the current situation. I do not think that we actually need to move it, but it is confusing if it sometimes is imported from text and sometimes from textpath.