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
deprecation attr #15310
deprecation attr #15310
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 didn't expect to see so much diff. I thought it would be a few lines to add a new attribute.
If you look at 12b5d46, then it's only a few lines. |
Can we separate the two into different PRs? 🙏 Also I don't think refactoring is necessary given that you already want to replace with yet another implementation down the road. |
Sure.
This code cleanup / improvement is cherry picked from #15301, which then just changes some more internals to use |
80b00bb
to
4f8c9fd
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.
Much cleaner, thanks! I'll let mhvk review since he requested it.
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.
Yes, this looks good! Should there be some test, though? E.g., I think it works on attributes indirectly, maybe worthwhile to test? I guess more generally: does it in fact work in your IDE?
I think you need to rebase to pick up RTD fix. |
Signed-off-by: nstarman <nstarman@users.noreply.github.com>
Signed-off-by: nstarman <nstarman@users.noreply.github.com>
5f3b51f
to
267112e
Compare
It might be worth suggesting to VSCode to assume that anything marked by |
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.
Looks good!
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.
Thanks!
Adds the
__deprecated__
attribute.Also cleans updeprecated
so that we don't re-define the helper functions every timedeprecated
is called.deprecated
still redefines all functions on every call.