access_url in RegTAP backward compatibility hacks

[msdemlei]

There is a warning now, though, that ought to tip people off when things
go wrong. See also #340; it does
not directly address the problem outlined there.

Relatedly, we now properly parse the arrays-in-strings we get back from
TAP. So far, an empty result yielded [""] in the array of, e.g., access_url.
We ought to have proper string arrays in VOTable one day.

[codecov]

Codecov Report

Merging #341 (e8d11b4) into main (197cd38) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #341      +/-   ##
==========================================
+ Coverage   78.28%   78.31%   +0.03%     
==========================================
  Files          46       46              
  Lines        5493     5501       +8     
==========================================
+ Hits         4300     4308       +8     
  Misses       1193     1193              

Impacted Files	Coverage Δ
pyvo/registry/regtap.py	78.94% <100.00%> (+0.70%)	⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[msdemlei]

While composing an answer to Bug #340, I noticed the problem raised there can reasonably be addressed by adding an access_url column that's the unique URL if it exists and None otherwise; that doesn't spoil new behaviour but falls back to what it was when possible. So, the second commit enables that.

[bsipocz]

I don't think a new method is needed at this point, the ultimate goal would be to eliminate the previous behaviour and raise the exception if the access_url is not unique.

[bsipocz]

Unfortunately I don't think this addresses the backward compatibility question, given that get_summary is new in this refactor and as far as I see is not called in any of the other methods to provide backward compatible behaviour.

Maybe the solution is to have a major version bump along with all the changes from the refactor?

[msdemlei]

Ok -- I've force-pushed to this branch, taking out the commit that added acess_url to to_summary's columns.

What's left I'd consider useful, in particular because this will now produce a much less confusing error message when there is no access URL at all (e.g., for authority or standards records; while pyvo itself doesn't do anything with them yet, user programs may have reason to do so, and they could turn up in result sets anyway).

So... I'm grateful for reviews.

[tomdonaldson]

Thanks @msdemlei. I do think these changes are useful, and while not addressing the access_url missing in to_table() aspect of #340 , I feel like this is sufficient.

[msdemlei]

On Thu, Jun 09, 2022 at 12:20:37PM -0700, Brigitta Sipőcz wrote: @bsipocz commented on this pull request. > @@ -196,20 +199,27 @@ def get_summary(self): This is mainly intended for interactive use, where people would like to inspect the matches in, perhaps, notebooks. + + This returns a column access_url for backwards compatibilty. Do Unfortunately I don't think this addresses the backward compatibility question, given that `get_summary` is new in this refactor and as far as I see is not called in any of the other methods to provide backward compatible behaviour.

You're right -- that was I thinko. I'll back out of this (not sure whether I'll find time for pyvo today, though). I think I'll leave the changes to access_url attribute, though; they're clearly an improvement either way.

Maybe the solution is to have a major version bump along with all the changes from the refactor?

I'd hope the changes are *largely* backwards-compatible enough to let us get away with a minor version without twisting semantic versioning too badly. The rationale would be "Code that uses registry.search as intended (and before get_summary we didn't encourage peeking inside the result sets) will keep working". The reason I'd be reluctant about a major version is that if we do that, we probably ought to have a few other breaking changes in, too; perhaps we should start collecting them (example: all AdhocSomethings should be renamed to DatalinkSomething or SODASomething).