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
DEP: rename io.votable.tree.Table to TableElement #15372
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.
|
Maybe send a memo out to astropy-dev ? I can imagine a lot of VO server code is not public and even if this is used, we won't know. 🤞 Thanks! |
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, @bsipocz. From a technical perspective, this seems straightforward and complete.
It is unlikely this class has external usage, but if it does the deprecation will give an opportunity to switch. Notice was also sent on the appropriate IVOA e-mail list: http://mail.ivoa.net/pipermail/apps/2023-September/001608.html
Doesn't this affect the following?
Should this go in first, then the other two after they adapt? |
I don't expect direct clash with the parquet PR, but still would like to have that go in first as it does some weird stuff in the tests to avoid the nameclash that this PR addresses (e.g. it does |
parquet PR is merged so now there is conflict. Please rebase again. Thanks! |
4249ce4
to
43a56a0
Compare
Do @tomdonaldson or @andamian want to do one more pass of reviews? |
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. Thanks!
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.
This still LGTM after the rebase.
Thanks, all! |
that has been renamed to tree.TableElement but not caught in astropy#15372
To fix #15358
Needs feedback from downstream users, too, though maybe there aren't many direct usage of the class (a quick look brought up only one in pyvo, elsewhere it may be even fewer)