-
-
Notifications
You must be signed in to change notification settings - Fork 53
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
BUG: fix StringIO uploads #626
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #626 +/- ##
=======================================
Coverage 82.31% 82.31%
=======================================
Files 72 72
Lines 7429 7429
=======================================
Hits 6115 6115
Misses 1314 1314 ☔ View full report in Codecov by Sentry. |
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 get red lights flashing when I see strings and bytes in parallel. I have not tried to ascertain that something will be doing the encoding (to utf-8?), and I note that that is not necessarily trivial for XML (which as encoding indicators inside: a charset attribute in the xml declaration, defaulting to utf-8 if not present).
Given that, I'd really love to have some regression test for that. I'd even bite the bullet and have a remote test for it...
Or is there a simple way to see that the byte/str mix here is unproblematic in this particular context?
My take on this is that in case of streams we just pass along what we got already on the class instance. Whether that was provided by the user or went through some validation is a separate question. That being said, I'm happy to add the tap upload test from astroquery as this one smoked the issue out: https://github.com/astropy/astroquery/blob/main/astroquery/alma/tests/test_alma_remote.py#L566 |
That remote test passes for both |
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.
Ok, I'm happy with Adrian's explanation, and I trust requests will be doing something robust, so let's go.
I still meant to add more tests (practically moving or copying the one from astroquery), but it slipped off the radar. |
This is a follow-up to #617 and does fix the issue I see in astroquery.