Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Deprecate globals using module-level__getattr__.#20733
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| except TypeError as exc: | ||
| cursord = {} # deprecated in Matplotlib 3.5. | ||
| @functools.lru_cache(None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| @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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
How would you want that to be formatplotlib/__init__.py, which also uses__getattr__ for__version__?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 thematplotlib.__init__.py__getattr__.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actually, I just put the comment in theif block.
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'llstill go through the lru_cache layer), but that should hopefully benegligible, and will mostly concern deprecated variables anyways.One *could* consider adding a helper API in `_api.deprecation` that justprovides the decoration with `@lru_cache` and generates theAttributeError message when needed, but that seems rather overkill.The change in deprecation.py allows one to skip `obj_name` and get amessage like "foo is deprecated..." rather than "The foo isdeprecated...".
PRmatplotlib#20733 added module-level `__getattr__` functions to severalmodules. All of the functions with the exception of the one in`matplotlib.style.core` had a terminal `raise AttributeError` tohandle unmatched attributes.The omission in `matplotlib.style.core` was probablyunintentional; it results in confusing and buggy behaviorsuch 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 affiliatedpackages. Seeastropy/astropy#12038.
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 repeattwice 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.
Onecould 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).