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
repr for VOTable is now clearer #14702
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.
|
Any objection for this one? |
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.
LGTM. Thanks @pllim.
Can we call this a bugfix though rather than a new feature? (misrepresenting is a bug in my read) |
Sure, please see #14906 |
s = repr(self.to_table()) | ||
if s.startswith("<Table"): | ||
s = "<VO" + s[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.
Looking at this again, now I wonder why the conditional?
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.
To guard against something else breaking unexpectedly because this is old code and I don't understand the whole stack in this subpackage. So I chose to be very explicit on when to overwrite the repr. Did this if break stuff for you?
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.
No break, I'm just about to do some more similar things in pyvo and spotted this as something odd
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.
If you are sure you don't need the if, you can remove it. I am just not 100% sure and don't have time to investigate. Thanks!
Description
This pull request is to fix #14700