Deprecate globals using module-level __getattr__.
#20733
Merged
+86
−49
Conversation
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
timhoffm
reviewed
Jul 25, 2021
| except TypeError as exc: | ||
| cursord = {} # deprecated in Matplotlib 3.5. | ||
|
|
||
| @functools.lru_cache(None) |
There was a problem hiding this comment.
Suggested change
| @functools.lru_cache(None) | |
| # module-level depredations | |
| @functools.lru_cache(None) |
Since we don't make a dedicated function which could give the code segment a name, let's at least use a comment as a standard convention for explaining what this is for. - Also for the other uses of this pattern.
There was a problem hiding this comment.
How would you want that to be for matplotlib/__init__.py, which also uses __getattr__ for __version__?
There was a problem hiding this comment.
For simplicity, you can leave that without a comment. Part of the intent of the comment is to easily see that the whole construct serves the deprecation and can be removed when expiring. That's not the case for the matplotlib.__init__.py __getattr__.
There was a problem hiding this comment.
Actually, I just put the comment in the if block.
timhoffm
approved these changes
Jul 26, 2021
This PR is mostly just to propose a pattern for defining module-level `__getattr__`s for deprecating globals. Relying on lru_cache rather than defining variables as `global` (see change in `__init__.py`) avoids having to repeat *twice* the variable name, and allows immediately returning its value without assigning it. It means later accesses will be very slightly slower (because they'll still go through the lru_cache layer), but that should hopefully be negligible, and will mostly concern deprecated variables anyways. One *could* consider adding a helper API in `_api.deprecation` that just provides the decoration with `@lru_cache` and generates the AttributeError message when needed, but that seems rather overkill. The change in deprecation.py allows one to skip `obj_name` and get a message like "foo is deprecated..." rather than "The foo is deprecated...".
9 tasks
lpsinger
added a commit
to lpsinger/matplotlib
that referenced
this pull request
Aug 10, 2021
PR matplotlib#20733 added module-level `__getattr__` functions to several modules. All of the functions with the exception of the one in `matplotlib.style.core` had a terminal `raise AttributeError` to handle unmatched attributes. The omission in `matplotlib.style.core` was probably unintentional; it results in confusing and buggy behavior such as: ```pycon >>> import matplotlib.style.core >>> if hasattr(matplotlib.style.core, '__warningregistry__'): ... del matplotlib.style.core.__warningregistry__ ... Traceback (most recent call last): File "<stdin>", line 2, in <module> AttributeError: __warningregistry__ ``` This causes problems in the unit tests for astropy affiliated packages. See astropy/astropy#12038.
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is mostly just to propose a pattern for defining module-level
__getattr__s for deprecating globals.Relying on lru_cache rather than defining variables as
global(seechange in
__init__.py) avoids having to repeat twice the variablename, and allows immediately returning its value without assigning it.
It means later accesses will be very slightly slower (because they'll
still go through the lru_cache layer), but that should hopefully be
negligible, and will mostly concern deprecated variables anyways.
One could consider adding a helper API in
_api.deprecationthat justprovides the decoration with
@lru_cacheand generates theAttributeError message when needed, but that seems rather overkill.
(See #20620 (comment), in response to which this was written:
I don't have any "magic" idea this time :-), I now think the #10735
isn't really worth it.)
The change in deprecation.py allows one to skip
obj_nameand get amessage like "foo is deprecated..." rather than "The foo is
deprecated...".
PR Summary
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).