Widen return type of pyplot.subplot_tool to Any#31997
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>
| `~matplotlib.widgets.SubplotTool` or None | ||
| The subplot tool window. Returns a `~matplotlib.widgets.SubplotTool` | ||
| for the widgets backend, a backend-native dialog (e.g. Qt) for other | ||
| backends, or `None` for backends using the toolmanager. |
There was a problem hiding this comment.
I'm new here, so take my comment with a grain of salt :)
The return value might still be too specific since "SubplotTool or None" doesn't include the Qt dialog case.
In the case of my work on the macosx backend, there's no way to return a "backend-native dialog", since that would be the raw NSWindowController.
Perhaps the Return type should include Any and the statement be something similar to:
Returns a
~matplotlib.widgets.SubplotTool,Nonefor backends using the toolmanager, or an implementation-specific value.
There was a problem hiding this comment.
Thanks — agreed the old wording was too narrow. I've generalized the description to "an implementation-specific value" so it covers the macosx case where there's no meaningful dialog to return.
I kept the type line as object or None rather than Any: a parallel review thread pushed the annotation to object | None (safer than Any, which turns off type checking for callers), and object still conveys "backend-defined, don't rely on the concrete type." I also updated the configure_subplots stub to object to match.
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
Widen the return annotation of
pyplot.subplot_toolandNavigationToolbar2.configure_subplotsfromSubplotTool | NonetoAny. 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