TJ's implementation for issue 212: TAP examples #220
Conversation
|
Assigning milestone 1.1. I think this looks good! I'm going to ask if @funbaker has any thoughts on the XML parts, since I'm pretty clueless in that regard, I just follow the pattern. I'm also not sure what the test failure is, but it doesn't look related? |
|
I think the test failure is due to astropy/astropy#9505 which might be pushed to astropy release 4.1, based on the discussion in the Astropy Slack astroquery channel. |
|
The XML Parser Stuff is fine. I suspect that astropy PR for making the test fail as well. |
|
The test updates should be compatible with new and old versions of astropy. Once the dust settles, Astropy 4.1+ will not put |
|
@tomdonaldson pyvo's tests test explicit for a byte string. hence i said it needs to be fixed in the pyvo tests. |
|
@funbaker Yes, I totally agree. My point was that the pyvo tests should be written to pass whether the Astropy version is <4.1 or >=4.1. |
|
At my level of experience, I don't want to mess with other people's tests, so I guess I'll rebase when somebody else fixes that. |
|
@trjaffe no worries, thats what code reviews are for. |
|
Unrelated comment, but I saw the notebook for the diff here: Do you test the notebooks? If not then consider (not just here, but for the other notebooks as well) massaging the examples into code snippet into the narrative docs, so the doctesting is actually picking them up, rather than having notebooks. |
|
Sorry this one fell off my radar, can we rebase and I think that should get the build to work? |
Don't I have to wait for PR #218 to be approved and merged? I had understood that it has some fixes to the tests in it. |
Codecov Report
@@ Coverage Diff @@
## master #220 +/- ##
==========================================
+ Coverage 72.19% 72.20% +0.01%
==========================================
Files 42 42
Lines 4478 4498 +20
==========================================
+ Hits 3233 3248 +15
- Misses 1245 1250 +5
Continue to review full report at Codecov.
|
| @@ -138,6 +141,33 @@ def tables(self): | |||
| vosi.parse_tables(response.raw.read), tables_url) | |||
| return self._tables | |||
|
|
|||
| @property | |||
| def examples(self): | |||
There was a problem hiding this comment.
Sorry to chime in here so late in the game, but the examples end point is not part of the standard, is it? If it's not ,shouldn't it be available under a new submodule (extensions)? I know of a few other TAP features that are not yet part of the standard but used by at least a couple of service providers but I was reluctant to add them to the core library.
There was a problem hiding this comment.
Looks like it's in 1.1 but not in 1.0? (I usually only look at the 1.1) from the new DALI? http://www.ivoa.net/documents/TAP/20190927/REC-TAP-1.1.html section 2
There was a problem hiding this comment.
Indeed, I must have looked at 1.0. What probably confused me was that I haven't seen the capabilities standard ID for examples (ivo://ivoa.net/std/DALI#examples). My read of the standard is that unlike the /tables end point, one for examples is actually specified in the capabilities of the service and could end up being different from baseURL/examples
|
On Tue, May 05, 2020 at 04:15:14PM -0700, andamian wrote:
+ @Property
+ def examples(self):
Sorry to chime in here so late in the game, but the `examples` end
point is not part of the standard, is it? If it's not ,shouldn't it
be available under a new submodule (`extensions`)? I know of a few
other TAP features that are not yet part of the standard but used
by at least a couple of service providers but I was reluctant to
add them to the core library.
The examples endpoint is, indeed, is a later addition to, well, TAP,
or really anthing else (via DALI), but it's in use for plenty of TAP
1.0 services.
It's also completely innocent to have it in a generic client, as both
1.0 and 1.1 behave identically: Either they serve proper examples, in
which case there's a 200 response with a text/xhtml body, or they
don't, in which case it gives a 404.
|
|
Okay this is green and has been approved and #218 got merged in, so I'm going to take this as part of 1.1 |
This appears to work for all the services I tried, which included those using XHTML or HTML. Either way, it's just using the xml.etree.ElementTree parser, which is a standard library not a new dependency. So I think this should be OK, right?
Second question: the response to /examples is optional. This implementation returns a simple list of TAPService objects. In the case that the response is 404 (allowed), it returns an empty list rather than None. Is this what we want?
(Sorry about the diffs in the notebook. The format of the whole thing apparently had to be updated according to my Jupyter.)