Widen return type of pyplot.subplot_tool to object | None#31997
Widen return type of pyplot.subplot_tool to object | None#31997karlhillx wants to merge 6 commits into
Conversation
…re_subplots to Any Per timhoffm on matplotlib#31979. The Qt backends return QDialog subclasses and the toolmanager branch already returns None, so the prior annotation was misleading. Update docstring to describe the backend-dependent return value. Also drop the now-unused TYPE_CHECKING import of SubplotTool and alphabetize the typing import. Closes matplotlib#31979
|
Thank you for opening your first PR into Matplotlib! If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks. We also ask that you please finish addressing any review comments on this PR and wait for it to be merged (or closed) before opening a new one, as it can be a valuable learning experience to go through the review process. You can also join us on discourse chat for real-time discussion. For details on testing, writing docs, and our review process, please see the developer guide. We strive to be a welcoming and open project. Please follow our Code of Conduct. |
There was a problem hiding this comment.
Pull request overview
This PR updates Matplotlib’s typing/doc surface to reflect that pyplot.subplot_tool() / NavigationToolbar2.configure_subplots() return backend-dependent objects (e.g., a SubplotTool on classic backends vs a native dialog on Qt), rather than always returning a SubplotTool.
Changes:
- Widened the return annotation of
pyplot.subplot_toolto a backend-agnostic type. - Widened the return annotation of
NavigationToolbar2.configure_subplotsin the type stubs. - Updated the
subplot_tooldocstring to describe backend-dependent return values.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lib/matplotlib/pyplot.py | Adjusts subplot_tool() return annotation and updates its docstring to describe backend-dependent returns. |
| lib/matplotlib/backend_bases.pyi | Updates the stubbed return type of NavigationToolbar2.configure_subplots. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Address review feedback: keep the public return annotation as object | None (safer than Any, which disables type checking for callers) and match the configure_subplots stub to object. Rewrite the Returns description to cover backends without a native dialog (e.g. macosx) as an implementation-specific value. Drop the unnecessary top-level Any import move.
5c8ee32 to
e1e96ec
Compare
eeshsaxena
left a comment
There was a problem hiding this comment.
The change itself looks right - I confirmed that after removing the TYPE_CHECKING import, SubplotTool has no remaining references in pyplot.py (the only three occurrences were the import, this annotation, and the docstring line, all updated here), and there's no pyplot.pyi that also annotates subplot_tool, so nothing is left dangling. The docstring rewrite describing the backend-dependent return is a nice improvement.
Two small things:
-
Title vs. code: the PR title and description say the return type is widened to
Any, but the code actually usesobject(object | Noneinpyplot.py,objectinbackend_bases.pyi). Those aren't equivalent -Anyopts out of type checking, whileobjectis the restrictive top type - so it's worth aligning the title with what landed (which, given "Direction per timhoffm on #31979", looks likeobjectis the intended choice). Just so the merge-commit message isn't misleading. -
object | Noneis redundant: sinceNoneTypeis a subclass ofobject,object | Nonenormalizes to justobjectfor a type checker.backend_bases.pyialready uses plainobjectforconfigure_subplots, sosubplot_toolcould match that for consistency - thoughobject | Nonedoes document theNonecase more explicitly, so this is minor either way.
Otherwise this looks good.
The .pyi stub now matches pyplot.py's object | None annotation, documenting the None case explicitly per the maintainer's request.
|
Thanks @eeshsaxena — both points addressed: Title/description: Updated. The title and body now say
|
|
Thanks for the quick turnaround. Both look resolved to me:
Looks good from my side - deferring to the maintainers for the final review. |
Widen the return annotation of
pyplot.subplot_toolandNavigationToolbar2.configure_subplotsfromSubplotTool | Nonetoobject | None. The Qt backends return aQDialogand the toolmanager branch already returnsNone, so the annotations were misleading. Direction per timhoffm on #31979. Docstring updated to describe the backend-dependent return value.Also drop the now-unused
TYPE_CHECKINGimport ofSubplotTool, and alphabetize thetypingimport.Closes #31979