[XMatch] fix: upload of two local tables is now possible#3116
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files☔ View full report in Codecov by Sentry. |
bsipocz
left a comment
There was a problem hiding this comment.
Thank you for the fix!
Local only tests for this is fully reasonable.
| fp = StringIO() | ||
| cat.write(fp, format='ascii.csv') | ||
| fp.seek(0) | ||
| kwargs['files'].update({catstr: (f'cat{cat_index}.csv', fp.read())}) |
There was a problem hiding this comment.
I suppose you don't have to regenerate cat{cat_index} here, but reuse it from L127 as f'{catstr}', however this is a ridiculous level of nitpicking, so I'll just go ahead and merge the fix instead :)
There was a problem hiding this comment.
Oh, yes it was better than what I did. Don't hesitate to change something if it happens again in an other PR, I won't be offended 🙂
Hello astroquery 🙂
In the files dictionary sent to XMatch, the second table was written over the first one, resulting in a confusing "missing cat1" error even if the user did give a cat1. This fixes #3115 , closes #1687 and closes #1786
I did not add a remote test, only a local one. Is it ok?