Skip to content

MAINT: deprecate validCap, validJoin #18817

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 1 commit into from
Nov 7, 2020
Merged

MAINT: deprecate validCap, validJoin #18817

merged 1 commit into from
Nov 7, 2020

Conversation

brunobeltran
Copy link
Contributor

@brunobeltran brunobeltran commented Oct 26, 2020 •
edited
Loading

PR Summary

Needed for #18544 (GSOD). Some new machinery was needed to add the deprecation while keeping the existing API (namely, the ability to access validJoin as a class property directly, i.e. Line2D.validJoin), so I pulled the necessary code out into this PR.

These properties already have long-existing comments marking them as "only there for backwards compatibility", and deprecation was initially suggested by @QuLogic.

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • [N/A] New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • [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).

@brunobeltran brunobeltran force-pushed the deprecate_valid_joincap branch from b884fe9 to c505b7b Compare October 26, 2020 16:39
@brunobeltran brunobeltran mentioned this pull request Oct 26, 2020
Merged
5 tasks
QuLogic
QuLogic reviewed Oct 27, 2020
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.

We don't normally put a full version number with micro component.

@QuLogic
Copy link
Member

QuLogic commented Oct 27, 2020

I would also maybe wait until deprecation machinery is properly exposed from _api, so as to not introduce more cbook usage.

@brunobeltran brunobeltran force-pushed the deprecate_valid_joincap branch 3 times, most recently from 4f55d60 to 384d96a Compare October 27, 2020 20:41
@brunobeltran brunobeltran requested a review from QuLogic October 27, 2020 20:41
@brunobeltran
Copy link
Contributor Author

I would also maybe wait until deprecation machinery is properly exposed from _api, so as to not introduce more cbook usage.

Should be clean of any new references to cbook now.

@QuLogic
Copy link
Member

QuLogic commented Oct 29, 2020

Based on #18823, I'm not sure that's the way @timhoffm wants to go, but it's still not ready yet.

@brunobeltran brunobeltran force-pushed the deprecate_valid_joincap branch from 384d96a to d9674c3 Compare November 3, 2020 19:44
@brunobeltran
Copy link
Contributor Author

brunobeltran commented Nov 3, 2020 •
edited
Loading

@timhoffm Can you review whether this is the way you want _api.deprecation to be used moving forward? It sounds like otherwise, this might be ready?

@brunobeltran
Copy link
Contributor Author

Per the call, this should wait until I silence the deprecation warnings this emits in conf.py, specifically for autodoc.

@brunobeltran brunobeltran force-pushed the deprecate_valid_joincap branch 2 times, most recently from c6c3cd6 to aed162f Compare November 5, 2020 01:16
@brunobeltran
Copy link
Contributor Author

Extra warnings have been quenched in conf.py. Thankfully warnings allows enough specificity to explicit ask it to ignore just MatplotlibDeprecationWarnings coming specifically from the sphinx.util.inspect module.

QuLogic
QuLogic reviewed Nov 5, 2020
View reviewed changes
lib/matplotlib/_api/deprecation.py Outdated
@@ -16,6 +16,42 @@
import warnings


# has to be here (instead of cbook) to prevent circular import, since
# deprecation.deprecated is *used* during cbook's import
class _classproperty:
Copy link
Member

Choose a reason for hiding this comment

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

Since _api is already private.

Suggested change
class _classproperty:
class classproperty:

Also, this isn't actually a deprecation; does it belong in this file, or in _api/__init__.py?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of the functions in _api.deprecation are still underscore delimited for direct importing into cbook.deprecation, happy to fix this one as long as you guys (@timhoffm) are cool with me sending in another PR to do this to the rest of the methods in _api, since I would feel gross if we end up with some random combination of underscores and no underscores.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, almost missed your other question. It should be in __init__ now that deprecation isn't in cbook there's no more import-time circular stuff that can't be avoided by a local import.

lib/matplotlib/_api/deprecation.py Outdated
@@ -16,6 +16,42 @@
import warnings


# has to be here (instead of cbook) to prevent circular import, since
Copy link
Member

Choose a reason for hiding this comment

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

It will never be in cbook again; should this say _api, or is the comment now irrelevant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sorry, this comment was from before the _api changes, and should definitely read _api, not cbook.

@brunobeltran brunobeltran force-pushed the deprecate_valid_joincap branch 4 times, most recently from ab249b4 to 46866fb Compare November 5, 2020 15:11
QuLogic
QuLogic approved these changes Nov 7, 2020
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.

Small docstring typo.

@brunobeltran brunobeltran force-pushed the deprecate_valid_joincap branch from 46866fb to 0b71fe0 Compare November 7, 2020 01:35
@QuLogic QuLogic added this to the v3.4.0 milestone Nov 7, 2020
@QuLogic QuLogic merged commit 97115ae into matplotlib:master Nov 7, 2020
@jklymak jklymak mentioned this pull request Dec 8, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuLogic QuLogic QuLogic approved these changes

@timhoffm timhoffm timhoffm approved these changes

Assignees
No one assigned
Labels
Maintenance
Projects
None yet
Milestone
v3.4.0
Development

Successfully merging this pull request may close these issues.
3 participants
@brunobeltran @QuLogic @timhoffm