Skip to content

MNT: Deprecate docstring - #22148

Merged
QuLogic merged 2 commits into
matplotlib:mainfrom
oscargus:deprecatedocstring
Mar 15, 2022
Merged

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

Conversation

@oscargus

@oscargus 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
oscargus force-pushed the deprecatedocstring branch 2 times, most recently from da97ff3 to b7eb392 Compare January 7, 2022 21:41
@oscargus

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

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
oscargus force-pushed the deprecatedocstring branch 2 times, most recently from 3d461d9 to 945ccfd Compare January 8, 2022 13:08
@oscargus

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

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

oscargus commented Jan 10, 2022

Copy link
Copy Markdown
Member Author

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

QuLogic
QuLogic reviewed Jan 14, 2022
View reviewed changes

@QuLogic 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.

Some of this seems unrelated to docstring.

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

oscargus commented Jan 16, 2022

Copy link
Copy Markdown
Member Author

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

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

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

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
oscargus marked this pull request as draft February 25, 2022 08:41
@oscargus
oscargus marked this pull request as ready for review February 28, 2022 15:40
@QuLogic

QuLogic commented Feb 28, 2022

Copy link
Copy Markdown
Member

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

@oscargus

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

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

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
QuLogic merged commit 223b2b1 into matplotlib:main Mar 15, 2022
@oscargus
oscargus deleted the deprecatedocstring branch March 15, 2022 21:11
@oscargus oscargus mentioned this pull request Mar 21, 2022
Closed
8 of 13 tasks
@ksunden ksunden mentioned this pull request Sep 19, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@timhoffm timhoffm timhoffm approved these changes
@QuLogic 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

@oscargus @timhoffm @QuLogic @anntzer