Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding RegistryResults and RegistryResource to the registry API #608

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

msdemlei
Copy link
Contributor

This has been found necessary in the context of global discovery (PR #470), but since users are actually handling instances of these classes quite a bit, it seems wise to bless them as parts of the API.

@msdemlei msdemlei mentioned this pull request Oct 10, 2024
@msdemlei
Copy link
Contributor Author

Oh, there is a CI failure we might want to fix while we're at it:

157 .. doctest-remote-data::
158
159   >>> from astropy.coordinates import SkyCoord
160   >>> registry.search(registry.Freetext("Wolf-Rayet"),
Differences (unified diff with -expected +actual):
    @@ -5,3 +5,3 @@
     ------------------------------- ...
     ivo://cds.vizier/j/a+a/688/a104 ...
    -   ivo://cds.vizier/j/aj/166/68 ...
    +  ivo://cds.vizier/j/apj/938/73 ...

I am a bit surprised that a resource vanishes here (apparently, its spatial coverage changed), but that new resources come up here is fairly expectable. if the matching language is rich enouch, I'd say "make sure ivo://cds.vizier/j/a+a/688/a104 is in the list" would be fine, else perhaps we should ignore the output?

@bsipocz bsipocz added this to the v1.6 milestone Oct 10, 2024
Copy link

codecov bot commented Oct 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.85%. Comparing base (029980e) to head (4706df9).
Report is 26 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #608      +/-   ##
==========================================
+ Coverage   81.70%   81.85%   +0.15%     
==========================================
  Files          69       70       +1     
  Lines        7093     7160      +67     
==========================================
+ Hits         5795     5861      +66     
- Misses       1298     1299       +1     

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

@bsipocz
Copy link
Member

bsipocz commented Oct 10, 2024

if the matching language is rich enouch, I'd say "make sure ivo://cds.vizier/j/a+a/688/a104 is in the list

We should be able to do a partial matching with an ellipse after the first result line. If that doesn't work, IGNORE_OUTPUT is indeed the way to go.

@@ -15,4 +15,5 @@
"Freetext", "Author",
"Servicetype", "Waveband", "Datamodel", "Ivoid", "UCD",
"Spatial", "Spectral", "Temporal",
"choose_RegTAP_service", "RegTAPFeatureMissing"]
"choose_RegTAP_service", "RegTAPFeatureMissing",
"RegistryResults", "RegistryResource",]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These need to be imported, too.

And that should fix the traceback in the docs build.

@bsipocz
Copy link
Member

bsipocz commented Oct 10, 2024

Ignore the changelog and milestone check statuses, I'm still thinking about which release to put this in and if it can be joined with other API cleanup PRs.

This has been found necessary in the context of global discovery, but
since users are actually handling instances of these classes quite a
bit, it seems wise to bless them as parts of the API.
Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Markus!

@bsipocz bsipocz merged commit e701754 into astropy:main Oct 11, 2024
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants