Skip to content

MNT: Deprecate docstring #22148

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Mar 15, 2022
Merged

MNT: Deprecate docstring #22148

merged 2 commits into from
Mar 15, 2022

Conversation

oscargus
Copy link
Member

@oscargus oscargus commented Jan 7, 2022 •
edited
Loading

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
Copy link
Member Author

oscargus commented Jan 7, 2022

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
Copy link
Member

timhoffm commented Jan 8, 2022 •
edited
Loading

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
Copy link
Member Author

oscargus commented Jan 8, 2022

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 added this to the v3.6.0 milestone 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
Copy link
Member

timhoffm commented Jan 9, 2022

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

@oscargus
Copy link
Member Author

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

QuLogic
QuLogic reviewed Jan 14, 2022
View reviewed changes
Copy link
Member

@QuLogic QuLogic left a comment

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.

@oscargus
Copy link
Member Author

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

@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
Copy link
Member Author

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

@oscargus
Copy link
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
Copy link
Member

QuLogic commented Feb 28, 2022

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

@oscargus
Copy link
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
Copy link
Member Author

oscargus commented Mar 6, 2022

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
Copy link
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
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 Maintenance
Projects
None yet
Milestone
v3.6.0
Development

Successfully merging this pull request may close these issues.
4 participants
@oscargus @timhoffm @QuLogic @anntzer