Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
[TYP] Type changes from running against Pandas#26883
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
Apparently stubtest doesn't like that I put |
Uh oh!
There was an error while loading.Please reload this page.
@@ -1,10 +1,12 @@ | |||
from typing import TypeVar | |||
from ._axes import * | |||
from ._axes import Axes as Subplot | |||
from ._axes import Axes as Axes |
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.
Since that's the only thing in there, should we change the import in the.py
file, too?
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.
I mean, logically yes, but technically at runtime more gets imported there, just not things weintend to re-export.
I was being a bit conservative here andonly changing stub behavior (especially since the stubsdidn't pick upSubplot
as being exported, when itshould be)
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.
I actually think__init__.py
files (broadly) are good candidates for inlining type hints and just not having separate files: they are short, contain little to no actual function definitions, it's mostly imports and__all__
(which I'm also in favor of adding in more places)
Uh oh!
There was an error while loading.Please reload this page.
fe9a8b0
to0c6a3b0
Comparecontainers: list[Container] | ||
callbacks: CallbackRegistry | ||
child_axes: list[_AxesBase] |
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.
Looking at__clear
, I see alsolegend_
andtitle
that aren't here; should they be added as well? (The former appears inaxes.pyi
onAxes
)
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.
I'd say those ones (and perhaps several of the existing attrs) should be made private (title
there is already_[left|right]_title
whichare private, though the getters/setters are defined in_axes.py
, not_base.py
, for some reason, and not defined forsecondary_axes
, even though the attrs exist)
But I'll add them since they are by naming convention public (though I will also note that none of these attrs (aside fromdataLim
andviewLim
) actually appear in the rendered docs, though several (trans[Axes,Scale,Limits,Data]
come to mind)are referred to by examples, etc. Is that something wewant to include in the rendered docs? cc@timhoffm, this is reminiscent of the discussions regardingplt.Axes
/plt.Figure
, but is different in that these are not imports and are instance attributes)
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.
ax.title
is used in several tests as well as thelifecycle
tutorial andtight_layout_guide
axes.legend_
is used outside of theaxes
module only inbackend/qt_editor/figureoptions.py
as far as I can tell through quick greps (and never in documentation)
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.
LGTM, if@timhoffm is good with the additional items.
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.
The instance attributes are technically public. Documenting the types is consistent with our current public API. It’s a separate question, whether they should be public, and IMHO many shouldn’t. But that has to go through the deprecation cycle.
…883-on-v3.8.xBackport PR#26883 on branch v3.8.x ([TYP] Type changes from running against Pandas)
Uh oh!
There was an error while loading.Please reload this page.
PR summary
This is a few typing related changes identified by running against Pandas
ArtistList
generic so that Axes properties can be type narrowed__clear
, rather than__init__
directly, so they are available on all Axes, but were missed when looking for attrsAdd a__all__
toaxes/__init__.pyi
axes/__init__.pyi
handles imports/re-exportingSubplot
_AxisWrapper
to type hint forTickHelper.set_axis
AxisWrapper
is set in Polar code and flags if absent herex.set_axis(x.axis)
type checks as OKAny
PR checklist