Skip to content

ENH: Declaratively define new-style standard IDs in Servicetype constraint#744

Merged
msdemlei merged 3 commits intoastropy:mainfrom
stvoutsin:issue-707
Apr 2, 2026
Merged

ENH: Declaratively define new-style standard IDs in Servicetype constraint#744
msdemlei merged 3 commits intoastropy:mainfrom
stvoutsin:issue-707

Conversation

@stvoutsin
Copy link
Copy Markdown
Contributor

The Servicetype constraint handled sia2, hats, and hips through separate elif branches, raw SQL strings in extra_fragments and a fragile string-comparison.

This adds a module-level NEW_STYLE_STDIDS dict mapping the standard shorthand keys to its LIKE patterns and replaces extra_fragments with a like_patterns set.

With this approach adding a new new-style standard now requires a single line in NEW_STYLE_STDIDS.

Does this address #707 the way you were thinking @msdemlei ?

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 24, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.54%. Comparing base (66e0cb2) to head (f14fabb).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #744      +/-   ##
==========================================
+ Coverage   79.52%   79.54%   +0.01%     
==========================================
  Files          91       91              
  Lines       10293    10293              
==========================================
+ Hits         8186     8188       +2     
+ Misses       2107     2105       -2     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bsipocz bsipocz added this to the v1.9 milestone Mar 25, 2026
Copy link
Copy Markdown
Contributor

@msdemlei msdemlei left a comment

Choose a reason for hiding this comment

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

This is much better than it was before, thanks.

However, now that I look I'm pretty sure the hips identifier is not what we want. What's in there right now will find hipslists, which is not something that's useful with pyvo at this point.

The pattern should be for hipses themselves and hence: ivo://ivoa.net/std/hips#hips-1.%.

Since this probably hasn't been in use much, I'd be fine changing this with this PR and much commenting. If you don't change it right here, let's make another PR for that right away before people become confused.

I also eye the hats identifier with suspicion. The HATS note doesn't say anything about capability ids yet; let's take that debate to the registry list; HATS folks: you should go there anyway, because once you are writing ivo://ivoa.net/std/hats, there should something corresponding to that in the registry, and we can help you with that.

@stvoutsin stvoutsin marked this pull request as ready for review March 25, 2026 18:01
@stvoutsin
Copy link
Copy Markdown
Contributor Author

This is much better than it was before, thanks.

However, now that I look I'm pretty sure the hips identifier is not what we want. What's in there right now will find hipslists, which is not something that's useful with pyvo at this point.

The pattern should be for hipses themselves and hence: ivo://ivoa.net/std/hips#hips-1.%.

Since this probably hasn't been in use much, I'd be fine changing this with this PR and much commenting. If you don't change it right here, let's make another PR for that right away before people become confused.

I also eye the hats identifier with suspicion. The HATS note doesn't say anything about capability ids yet; let's take that debate to the registry list; HATS folks: you should go there anyway, because once you are writing ivo://ivoa.net/std/hats, there should something corresponding to that in the registry, and we can help you with that.

Thanks for reviewing the changes. I've updated accordingly, removing HATS for now until the note adds capability ids and changing hiplist to hips-1. Let me know if you spot anything else needing change and in the meantime I will also contact folks involved with writing the HATS note about amending the note accordingly.

Comment on lines -431 to -433
elif std == 'hats':
self.extra_fragments.append(
"standard_id like 'ivo://ivoa.net/std/hats#hats-%'")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is now a breaking change, with this change we remove functionality that handles hats resources, please don't remove it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok I've reinstated and have also contacted the HATS folks to look into registration of the standard id.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yes, I promised writing up a paragraph about it in the note 😄

@stvoutsin stvoutsin marked this pull request as draft March 25, 2026 19:24
@stvoutsin stvoutsin marked this pull request as ready for review March 25, 2026 22:41
@msdemlei
Copy link
Copy Markdown
Contributor

msdemlei commented Mar 26, 2026 via email

Copy link
Copy Markdown
Contributor

@msdemlei msdemlei left a comment

Choose a reason for hiding this comment

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

Thanks!

@msdemlei msdemlei merged commit c347d5c into astropy:main Apr 2, 2026
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.

3 participants