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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DEPR: deprecate importing ErfaError and ErfaWarning from astropy.utils.exceptions #15777
Conversation
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
👋 Thank you for your draft pull request! Do you know that you can use |
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.
I think you are right that it is simply that module-level __getattr__
didn't exist yet - it is a very nice addition!
Just one comment with a further simplification in-line.
astropy/utils/exceptions.py
Outdated
stacklevel=1, | ||
) | ||
|
||
if name == "ErfaError": |
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.
I think this can just be
import erfa
return getattr(erfa, name)
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.
thank you ! I was initially going to use importlib
to do this dynamically but thought a dumb implementation was just as good, I missed that I could use getattr
(ironic given that I was implementing __getattr__
...)
55f02d0
to
d0bb9d8
Compare
@@ -0,0 +1 @@ | |||
Deprecate importing ``ErfaError`` and ``ErfaWarning`` from ``astropy.utils.exceptions``. |
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.
Why not say in the change log entry where the error and warning should be imported from?
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.
done !
d0bb9d8
to
2716250
Compare
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.
Happy that worked, and good addition to the changelog entry.
p.s. Be sure to mention your improvements in #13821 - I like your approach of just removing minor pain points so we can avoid more major refactoring (or make it easier). |
Ah, I see you already did add a note in #13821 (comment) - thanks! |
"in version 6.1 and will stop working in a future version. " | ||
f"Instead, please use\nfrom erfa import {name}\n\n", | ||
category=AstropyDeprecationWarning, | ||
stacklevel=1, |
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.
It's usually recommended to atleast use stacklevel=2
in these warnings (ref python/cpython#88462)
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.
True, but in the context of a module level __getattr__
, and from my quick experiments, it doesn't seem to make any difference.
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.
I think this is OK as is, so will merge now. Thanks, @neutrinoceros!
@mhvk , please don't forget the milestone next time. I have added it here. |
Description
Follow up to #10329, and in response to #13821 (comment)
This feels a bit too easy, so perhaps I'm missing something ? I'm considering that maybe it wasn't done 3 years ago because PEP 562 wasn't usable yet. This reasoning seems plausible since #10329 went into astropy 4.1, which was the last version to support Python 3.6 (PEP 562 landed in 3.7)
ping @mhvk