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

Handle custom terms in datalink.bysemantics again. #299

Merged
merged 1 commit into from
Feb 15, 2022

Conversation

msdemlei
Copy link
Contributor

@msdemlei msdemlei commented Feb 9, 2022

This is done by extracting non-core terms -- really, anything with a URI
that starts with something that is not the core URI -- and excepting
it from our normalisation.

This would fix #298.

@codecov
Copy link

codecov bot commented Feb 9, 2022

Codecov Report

Merging #299 (cf3c458) into main (b5b33e8) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #299      +/-   ##
==========================================
+ Coverage   75.50%   75.53%   +0.02%     
==========================================
  Files          44       44              
  Lines        5128     5134       +6     
==========================================
+ Hits         3872     3878       +6     
  Misses       1256     1256              
Impacted Files Coverage Δ
pyvo/dal/adhoc.py 66.58% <100.00%> (+0.48%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5b33e8...cf3c458. Read the comment docs.

CHANGES.rst Outdated
@@ -1,6 +1,8 @@
1.3 (unreleased)
================

- pyvo deals with non-core terms in datalink.bysemantics again.
This would fix issue #298.
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to append the PR number ([#299]) to make the validation happy. If this is an issue that requires quick release, we should do a minor release - 1.2.2. @tomdonaldson - do you agree?

if isinstance(semantics, str):
semantics = [semantics]
semantics = [term.lstrip("#") for term in semantics]

core_terms, other_terms = [], []
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to use lists instead of sets?

This is done by extracting non-core terms -- really, anything with a URI
that starts with something that is not the core URI -- and excepting
it from our normalisation.
@msdemlei
Copy link
Contributor Author

msdemlei commented Feb 11, 2022 via email

@msdemlei
Copy link
Contributor Author

I've asked the bug reporter about a verifier, and he offered

{{{
import pyvo
datalink_url = 'http://archive.eso.org/datalink/links?ID=ivo://eso.org/ID?SPHER.2016-09-26T03:04:09.308'
datalink = pyvo.dal.adhoc.DatalinkResults.from_result_url(datalink_url)
semantics = 'http://archive.eso.org/rdf/datalink/eso#calSelector_raw2master'
raw2master_url = next(datalink.bysemantics( semantics )).access_url
print(raw2master_url)
}}}

This is looking good. Hence I'd say this is fine to merge.

Is there anything I can do to get this into a release as fast as
possible such that ESO's notebooks work again?

@andamian andamian merged commit 365e1ae into astropy:main Feb 15, 2022
@andamian
Copy link
Contributor

@msdemlei - thanks for the fix. We'll try to do the release in the next day or so.

@tomdonaldson tomdonaldson modified the milestones: v1.3, v1.2.2 Feb 17, 2022
@bsipocz bsipocz modified the milestones: v1.2.2, v1.3 Mar 7, 2023
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.

support for full-url semantics no longer provided, breaking existing code
4 participants