Adding support for DALI example continuation properties.

[msdemlei]

While this is a DALI feature, for now we only support examples in TAP, so that's where where this happens.

Supporting continuations is desirable now that TOPCAT shows them, too.

This thing makes we wonder whether the way we're exposing examples is
actually useful -- these TAPQuery objects are, I think, fairly unwieldy;
how are users expect to deal with them?

Frankly, I wonder whether we shouldn't simply have a method open_examples
that would simple do a webbrowser.open for the examples URI.

If we want to parse them out, I'd say we should have a way to expose the documentation, too, and perhaps somehow reflect the hierarchical structure of the continuations; I think TOPCAT does this fairly nicely (point a recent TOPCAT at http://dc.g-vo.org/tap); but again, perhaps there's very little utility parsing examples for interactive use in the first place.

[codecov]

Codecov Report

Merging #483 (89e04c4) into main (6405cb4) will decrease coverage by 0.08%.
The diff coverage is 70.00%.

❗ Current head 89e04c4 differs from pull request most recent head ac1366f. Consider uploading reports for the commit ac1366f to get more accurate results

@@            Coverage Diff             @@
##             main     #483      +/-   ##
==========================================
- Coverage   80.07%   80.00%   -0.08%     
==========================================
  Files          52       52              
  Lines        6059     6042      -17     
==========================================
- Hits         4852     4834      -18     
- Misses       1207     1208       +1     

Files	Coverage Δ
pyvo/dal/tap.py	73.44% <70.00%> (+0.21%)	⬆️

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[bsipocz]

I have a few very generic comments, and I do apologise about them as they don't really comment on new code, but were there forever and the lines just got moved around in this diff.

As for actual behaviour about examples, I don't have strong opinions either and would refer it to be decided by the others. However I suppose that could be done independently of this PR.

[bsipocz]

why do you make examples_uri positional only? I don't see any downside in spelling out argument names, and in cases of optional kwargs I in fact prefer them to be always spelt out.

[bsipocz]

Please us a more specific Exception. Is this e.g. a ValueError, or maybe just a warning?

[bsipocz]

can we be more specific about what exceptions we expect?

[msdemlei]

On Tue, Aug 29, 2023 at 03:48:05PM -0700, Brigitta Sipőcz wrote: > @@ -149,6 +149,38 @@ def tables(self): vosi.parse_tables(response.raw.read), tables_url) return self._tables + def _parse_examples(self, examples_uri, /, depth=0): why do you make ``examples_uri`` positional only? I don't see any

Uh, apologies -- that should have been a * in accordance with your #408. Fixed in commit 80f8a097.

+ def _parse_examples(self, examples_uri, /, depth=0): + """returns the TAP queries from a DALI examples URI. + """ + if depth > 5: + raise Exception("Suspecting endless recursion when" Please us a more specific Exception. Is this e.g. a `ValueError`, or maybe just a warning?

This can't be a warning; what this catches is continuations pointing at each other, and the box would go into an endless retrieval loop if I didn't break out. Really, I don't expect any actual user will ever see this, which is why I originally hadn't bothered to find a good exception type. Since this really addresses service deployer trying their service with pyVO, I'm now settling for DALServiceError (but I'll probably follow any other suggestion, too).

+ root = xml.etree.ElementTree.parse( + io.BytesIO(response.content)).getroot() + exampleElements = ***@***.***="query"]') + except Exception as ex: can we be more specific about what exceptions we expect?

I suppose we could; on the other hand, the purpose of this construct is to make sure that whatever fails in there bubbles up as a DALServiceError to the user, which I think is a reasonable case for a catch-all, no?

[bsipocz]

The devtest failure is unrelated and has been fixed in main already.

[bsipocz]

This looks all right to me, but would prefer to have someone knowing more about TAP examples leave an approval, too.

[zoghbi-a]

I had a quick look at this PR. I think this is useful.
As a user, I often find myself trying to find examples to use.

I wonder if it is more useful to return some description (from property="name" for instance) and the query as a text rather than a TAPQuery. I find this more useful because:
1- The user rarely wants to use the exact query, and more often they want to modify it before submitting it.
2- Some examples will fail anyway if they require uploads for instance.

I think this will be more consistent with the way TOPCAT does it, where you see a description (from property=name) and when selected, it just give you the query text so you can edit it.

[msdemlei]

On Wed, Oct 18, 2023 at 02:19:51PM -0700, Abdu Zoghbi wrote: I wonder if it is more useful to return some description (e.g. `property="name"`) and the query as text rather than a TAPQuery. I find this more useful because: 1- The user rarely wants to use the exact query, and more often they want to modify it before submitting it. 2- Some examples will fail anyway if they require uploads for examples.

I agree that using TAPQuery objects to expose the examples was perhaps not the greatest choice, in particular because it loses the (IMHO all-important) URI with human-readable information on the example. I don't think this PR is the place to fix this, though; while I don't believe anyone is *actually* doing anything with these TAPQuery objects, I've been wrong in these assessments before, and thus for basic continuation support I'd say we keep the existing interface. And then we ought to build a more useful API to this, perhaps keeping the existing examples property for a while in parallel. Would you file a bug to that effect?

[tomdonaldson]

This seems like a nice improvement to me. As mentioned in the discussion, there are other improvements possible, but I'm satisfied to have those addressed in future PRs.

[andamian]

All the comments are good points worth considering but this PR fills a gap in the existing functionality so it's a good addition.

[bsipocz]

With the approach of not letting the perfect be the enemy of the good, I go ahead and merge this, and welcome follow-up PRs with further improvements.

[bsipocz]

Thank you @msdemlei!