Replace list_services by list_interfaces

[ManonMarchand]

This PR addresses #521

The list_services method is removed and replaced by list_interfaces to prevent the instantiation of Service objects that wouldn't be used.

The repr of the class Interfaces (added in #505 so not impacting the released version) is now lighter/shorter.

[msdemlei]

On Mon, Feb 19, 2024 at 02:03:12AM -0800, Manon Marchand wrote: This PR addresses #521

Thanks -- just reading the diff I've not noticed anything I'd consider untoward. However, I still think we should use this opportunity to amend the output of describe(). There, it currently says: if len(self._mapping["access_urls"]) == 1: print("Base URL: " + self.access_url, file=file) else: print("Multi-capability service -- use get_service()", file=file)

…

-- and I think we should change that such that it says: Service Capabilities: * <description, with generic fallback> (Cone Search) * <description, with generic fallback> (Cone Search) * <description, with generic fallback> (TAP) (infrastructure capabilities like VOSI and *possibly* datalink should probably not be shown). Since we're now hitting the Registry for describe() anyway, I'd at least think hard whether this should refresh the information from RegTAP; the advantage would be that .describe output would be constant against Servicetype constraints, which would *probably* a minor advantage. The interfaces retrieval I could write myself when the rest is there, though.

[ManonMarchand]

What about something like :

>>> import pyvo as vo
>>> res = vo.registry.search(ivoid="ivo://CDS.VizieR/I/239")[0]
>>> res.describe()
The Hipparcos and Tycho Catalogues
Short Name: I/239
IVOA Identifier: ivo://cds.vizier/i/239
Access modes: conesearch, tap#aux, web
- tap#aux: http://tapvizier.cds.unistra.fr/TAPVizieR/tap
- webpage: http://vizier.cds.unistra.fr/viz-bin/VizieR-2?-source=I/239
- conesearch: http://vizier.cds.unistra.fr/viz-bin/conesearch/I/239/h_dm_com?,
description: Cone search capability for table I/239/h_dm_com (Double and
Multiples: Component solutions -COMP)
- conesearch: http://vizier.cds.unistra.fr/viz-bin/conesearch/I/239/hip_main?,
description: Cone search capability for table I/239/hip_main (The Hipparcos
Main Catalogue)
- conesearch: http://vizier.cds.unistra.fr/viz-bin/conesearch/I/239/solar_ha?,
description: Cone search capability for table I/239/solar_ha (Solar System
Annex: Astrometric catalogue)
- conesearch: http://vizier.cds.unistra.fr/viz-bin/conesearch/I/239/solar_t?,
description: Cone search capability for table I/239/solar_t (Solar System
Annex: Tycho astrometry/photometry)
- conesearch: http://vizier.cds.unistra.fr/viz-bin/conesearch/I/239/tyc_main?,
description: Cone search capability for table I/239/tyc_main (The main part of
Tycho Catalogue)

The Hipparcos and Tycho Catalogues are the primary products of the European
Space Agency's astrometric mission, Hipparcos. The satellite, which operated
for four years, returned high quality scientific data from November 1989 to
...

there's nothing when there is no description. By looking into a few records, there is no description when it is a bit obvious.

I don't print vosi, datalinks and soda. Do you think I should have kept soda?

[msdemlei]

On Mon, Feb 19, 2024 at 07:57:49AM -0800, Manon Marchand wrote: What about something like :

I think I like the output; I'd use subsequent_indent=' ' in the textwrap call so the continuation lines line up, though. Similarly nitpicking: I'd probably re-use is_vosi in ignoring interfaces for description, somewhat like this: if (interface.is_vosi or interface.standard_id.split("#")[0] in {'soda', 'datalink'}): continue It's probably a bit less mystifying, and I'm *a bit* worried about "str in otherstr" overmatching. Perhaps we should even have a method is_user_visible() in Interface that would encapsulate that whole visbility logic?

I don't print vosi, datalinks and soda. Do you think I should have kept soda?

As long as we don't return anything useful to users from get_service, I think it shouldn't be in describe. Thanks! [oh, I've not investigated the CI failures]

[ManonMarchand]

(The failure in the devdeps CI looks like something that changed in the dev version of numpy, so not related. There is also the changelog but I think that one is not up to me, looks like a missing label?)

I like the is_user_visible idea!

[msdemlei]

On Mon, Feb 19, 2024 at 10:21:51AM -0800, Manon Marchand wrote: I like the is_user_visible idea!

Let's keep it in mind. Or perhaps we should have the inverse: is_infrastructure. That sounds a bit clearer to me, and if better follows the code logic, which is "check in a list of things that don't make much sense for get_service". Each "not" in a logic is a potential brain teaser in my experience...

[ManonMarchand]

I did not like the "not vosi, not datalink..." approach, so I went with the keys of the "service_for_standardid" dictionary. This would need to be updated anyway if the services change. It should keep things easier to maintain.

[msdemlei]

I've left a few comments that boil down to matters of taste -- and I'm happy to have this merged in its current state, too.

[msdemlei]

On Tue, Feb 20, 2024 at 02:15:21AM -0800, Manon Marchand wrote: I did not like the "not vosi, not datalink..." approach, so I went with the keys of the "service_for_standardid" dictionary. This would need to be updated anyway if the services change. It should keep things easier to maintain.

Good point.

[ManonMarchand]

I think I addressed all your comments 🙂

[codecov]

Codecov Report

Attention: Patch coverage is 95.23810% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 80.38%. Comparing base (cbec016) to head (c2c38ee).

Files	Patch %	Lines
pyvo/registry/regtap.py	95.23%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #525      +/-   ##
==========================================
+ Coverage   79.71%   80.38%   +0.66%     
==========================================
  Files          52       52              
  Lines        6178     6189      +11     
==========================================
+ Hits         4925     4975      +50     
+ Misses       1253     1214      -39     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[msdemlei]

On Thu, Feb 22, 2024 at 06:42:17AM -0800, Manon Marchand wrote: @ManonMarchand commented on this pull request. + # a service is user visible if it has a corresponding service class + if self.standard_id is not None and self.standard_id != "": The remote data document tests fail without it. Looks like some non standard services have an empty string in self.standard_id. I did not investigate further than adding this condition.

Ok... the old pain of "" and None not being distinct in VOTables, I suppose. I wonder if I should debug this and (presumably) standardise on standard_id being None and never "", but for let's keep it like this, then.

[msdemlei]

On Thu, Feb 22, 2024 at 06:47:23AM -0800, Manon Marchand wrote: I think I addressed all your comments :slightly_smiling_face:

Let's go, then. @bsipocz, if you agree, would you merge? Thanks all around!

[bsipocz]

Let's go, then. @bsipocz, if you agree, would you merge? Thanks all
around!

Oh, of course, you don't need to wait for me to merge things that are reviewed and ready to go.

However, I added some changelog conflicts with the v1.5.1 release this morning, so will go ahead fix that conflict and then merge.

[bsipocz]

Thanks @ManonMarchand!