FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

MNT: Deprecate `docstring` by oscargus · Pull Request #22148 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

MNT: Deprecate docstring - #22148

Merged
QuLogic merged 2 commits into
matplotlib:mainfrom
oscargus:deprecatedocstring
Mar 15, 2022
ConversationCommits (2)ChecksFiles changed
Merged

MNT: Deprecate docstring#22148
QuLogic merged 2 commits into
matplotlib:mainfrom
oscargus:deprecatedocstring

Conversation

oscargus commented Jan 7, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

PR Summary

Related to #16181.

I removed (most) unused imports. I realize that this may be a bad idea because people may import from the incorrect place, especially for pyplot.py. However, it is probably better to comment and/or add pylint/flake8-comments that this is the case. So feel free to point out that it should be restored.

(I will fix any incorrect internal imports, in case there are more than those I have found. If nothing else, it can be a good way to improve the local code base.)

Will add release note when tests pass. Done

PR Checklist

Tests and Styling

  • [N/A] Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

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

oscargus force-pushed the deprecatedocstring branch 2 times, most recently from da97ff3 to b7eb392 Compare January 7, 2022 21:41

oscargus commented Jan 7, 2022

Copy link
Copy Markdown
Member Author

Is it just lazy test writing or is the code supposed to import from the "wrong" module?

Similarly, is it expected that it is enough to import from pyplot? (plt.Rectangle etc.)

timhoffm commented Jan 8, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Some of the unused imports in pyplot are indeed intentional for convenience, e.g. plt.cycler. I agree that the state is unclear and we should improve on that. I've opened #22149 to follow up. Please leave all unused pyplot imports in place for this PR.

oscargus force-pushed the deprecatedocstring branch 2 times, most recently from 3d461d9 to 945ccfd Compare January 8, 2022 13:08

oscargus commented Jan 8, 2022

Copy link
Copy Markdown
Member Author

OK! I have restored that (but kept changes in some of the tests to not use the pyplot import).

I also did the two-step commit procedure here.

timhoffm approved these changes Jan 8, 2022
View reviewed changes
Comment thread doc/api/next_api_changes/deprecations/22148-OG.rst Outdated
timhoffm added this to the v3.6.0 milestone Jan 8, 2022
timhoffm added API: changes Changes to the public API, typically requiring deprecation. Maintenance labels Jan 8, 2022
oscargus force-pushed the deprecatedocstring branch 2 times, most recently from 00d8591 to 454d7bc Compare January 8, 2022 16:13
oscargus changed the title Deprecated docstring. Deprecated docstring Jan 8, 2022
oscargus force-pushed the deprecatedocstring branch from 454d7bc to be9e590 Compare January 8, 2022 17:09
oscargus marked this pull request as ready for review January 8, 2022 18:31

timhoffm commented Jan 9, 2022

Copy link
Copy Markdown
Member

@oscargus something changed in codebase under this PR. Can you please rebase on the main branch? Thanks.

oscargus commented Jan 10, 2022

Copy link
Copy Markdown
Member Author

Another of the deprecations changed the same import statement. Fixed now.

QuLogic reviewed Jan 14, 2022
View reviewed changes

QuLogic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Some of this seems unrelated to docstring.

Comment thread lib/matplotlib/colorbar.py
Comment thread lib/matplotlib/sankey.py
oscargus commented Jan 14, 2022
View reviewed changes
Comment thread lib/matplotlib/text.py

oscargus commented Jan 16, 2022

Copy link
Copy Markdown
Member Author

OK! So now I think it should be properly sorted.

oscargus commented Jan 18, 2022
View reviewed changes
Comment thread lib/matplotlib/backend_bases.py Outdated
QuLogic reviewed Jan 19, 2022
View reviewed changes
Comment thread lib/matplotlib/docstring.py Outdated
oscargus force-pushed the deprecatedocstring branch 2 times, most recently from c1302d3 to 72a2a5f Compare February 24, 2022 21:25
oscargus mentioned this pull request Feb 24, 2022
Merged
6 tasks
oscargus marked this pull request as ready for review February 24, 2022 21:49

oscargus commented Feb 24, 2022

Copy link
Copy Markdown
Member Author

Finally got around to update this. I hope I have not introduced any of the issues...

oscargus commented Feb 25, 2022

Copy link
Copy Markdown
Member Author

I should have properly rebased instead of just resolving the conflicts in the web interface... Will do that.

oscargus marked this pull request as draft February 25, 2022 08:41
oscargus marked this pull request as ready for review February 28, 2022 15:40

QuLogic commented Feb 28, 2022

Copy link
Copy Markdown
Member

It doesn't seem like you did a rebase in the end?

oscargus commented Feb 28, 2022

Copy link
Copy Markdown
Member Author

It doesn't seem like you did a rebase in the end?

Hmm, you are right... Not sure how that happened as I am quite sure I did a rebase (as well)...

I'll give it a new go.

oscargus marked this pull request as draft February 28, 2022 23:05
oscargus force-pushed the deprecatedocstring branch 2 times, most recently from d692157 to 14881c7 Compare March 6, 2022 10:01
oscargus changed the title Deprecated docstring MNT: Deprecate docstring Mar 6, 2022

oscargus commented Mar 6, 2022

Copy link
Copy Markdown
Member Author

Still confused where the merge commit came from, but now it is properly rebased.

oscargus marked this pull request as ready for review March 6, 2022 10:03
oscargus force-pushed the deprecatedocstring branch from 37a605f to 52df591 Compare March 15, 2022 08:16

oscargus commented Mar 15, 2022

Copy link
Copy Markdown
Member Author

Now I am quite certain (more than last time...), that everything is updated.

QuLogic merged commit 223b2b1 into matplotlib:main Mar 15, 2022
oscargus deleted the deprecatedocstring branch March 15, 2022 21:11
oscargus mentioned this pull request Mar 21, 2022
Closed
8 of 13 tasks
ksunden mentioned this pull request Sep 19, 2023
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

timhoffm timhoffm approved these changes
QuLogic QuLogic approved these changes

Assignees

No one assigned

Labels

API: changes Changes to the public API, typically requiring deprecation. Maintenance

Projects

None yet

Milestone

v3.6.0

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL