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

resolve #90 (ObsCore example) and resolve #91 (relax content-type) #93

Merged
merged 7 commits into from
Feb 7, 2023

Conversation

pdowler
Copy link
Collaborator

@pdowler pdowler commented Feb 1, 2023

resolve #90 (ObsCore example)
resolve #91 (relax content-type in response headers)
make INFO element with standardID a MUST

Hopefully these are final changes before PR.

resolve ivoa-std#91 (relax content-type in response headers)
make INFO element with standardID a MUST
jd-au
jd-au previously approved these changes Feb 1, 2023
Copy link
Contributor

@jd-au jd-au left a comment

Choose a reason for hiding this comment

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

The changes look good to me.

@pdowler
Copy link
Collaborator Author

pdowler commented Feb 1, 2023

Will give other authors a chance to comment.

@Bonnarel
Copy link
Contributor

Bonnarel commented Feb 1, 2023 via email

@msdemlei
Copy link
Collaborator

msdemlei commented Feb 1, 2023 via email

@Bonnarel
Copy link
Contributor

Bonnarel commented Feb 1, 2023 via email

Copy link
Member

@mbtaylor mbtaylor left a comment

Choose a reason for hiding this comment

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

Generally agree but see comments. There should also be a note of this change in the Sec 5.1 change log.

DataLink.tex Outdated
a different format, the content-type header of the response MUST be
``application/x-votable+xml'' with the
``content'' parameter set to ``datalink'',
a different format, the content-type header of the response MAY be any value
Copy link
Member

Choose a reason for hiding this comment

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

Agree with Markus, MAY doesn't make sense here, should be SHOULD (or possibly MUST)

DataLink.tex Outdated
</RESOURCE>
\end{verbatim}

As of this version, the {links} response {\bf must} include this INFO element
Copy link
Member

Choose a reason for hiding this comment

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

Suggest replacing "As of this version" with "From version 1.1 of this standard". Otherwise the text is likely to remain in future revisions and be confusing.

Copy link
Member

Choose a reason for hiding this comment

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

Also: DALI 1.1 4.4.3 says "The standardID of a service specification is the IVOA resource identifier for the StandardsRegExt record not including capability-specific fragments." Does that mean that the value attribute in this example should be value="ivo://ivoa.net/std/DataLink" instead of value="ivo://ivoa.net/std/DataLink#links-1.0" ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, that's true of the standardID of the specification as used in a StandardsRegExt document. Here is is the standardID of the links endpoint itself, which is defined within that spec.

So I think it's OK.

@pdowler
Copy link
Collaborator Author

pdowler commented Feb 1, 2023

on MUST vs {\bf must}.... the document uses the latter form (or unformatted) everywhere else. I can make the change in the other direction if that's what we should be doing.

git blame didn't really help me figure out where this came from, but the section on conformance terms (eg MUST) was added by Mark. Otherwise we've all used unformatted and bold lowercase.

@msdemlei
Copy link
Collaborator

msdemlei commented Feb 2, 2023 via email

note: not global search and replace: only used where applicable to
something specified in this doc

some minor editorial tweaks I noticed along the way
@pdowler
Copy link
Collaborator Author

pdowler commented Feb 3, 2023

along with this, the CADC datalink service you get to by querying the ivo://cadc.nrc.ca/argus TAP service now support the applicable DataLink-1.1 features:

  • always include the standardID INFO element
  • accept both VOTable mimetypes in RESPONSEFORMAT and set that same value in response header
  • include 3 new optional fields
  • added descriptions for the new fields

example for public data:

https://ws.cadc-ccda.hia-iha.nrc-cnrc.gc.ca/caom2ops/datalink?ID=ivo://cadc.nrc.ca/IRIS?f212h000/IRAS-25um

For CADC, link_auth is always optional and link_authorized is used, will remove our custom "readable" once our UI is adapted to use link_authorized).

We have not added the code to populate content_qualifier yet but it is feasible to do it - just don't have time right now.

Copy link
Member

@mbtaylor mbtaylor left a comment

Choose a reason for hiding this comment

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

I think that defining the \rfc* macros with a trailing space is ... questionable.

I can see why you've done it, but (1) it's unusual practice, so people are likely to write "... \rfcmay\ ..." and get too much whitespace, also (2) you can't avoid extra whitespace if you want punctuation following them directly. You might argue that (2) is unlikely for a modal verb which sounds plausible ... but the "Conformance-related definitions" section up front contains text like 'The words "\rfcmust'', "\rfcshould'', ...' which end up including unwanted whitespace inside the quotes (and I also see at least one occurrence of '\rfcoptional.').

Also, ,can you summarise the content-type requirement changes in the change log in Section 5.1.

pdowler and others added 3 commits February 6, 2023 10:58
changed use of blinks command to use trailing backslash style
Removed usage of trailing backslashes after some \rfc* macros
where they are not followed by whitespace.

The point of the "\xxx\ " idiom is to prevent the whitespace
that follows a macro from being interpreted as simply the macro
name delimiter and thus swallowed.
Writing "\xxx\ " stops that happening by placing a whitespace macro
after the \xxx macro; if you write something like "\rfcoptional\." it
is interpreted as the "\rfcoptional" macro followed by the "\." macro,
which isn't what you want.

An alternative way to ensure that the macro is terminated without
swallowing following whitespace is to write "{\xxx}".
@pdowler
Copy link
Collaborator Author

pdowler commented Feb 6, 2023

Thanks Mark. Unsufficient latex-fu on my part :-)

@pdowler pdowler merged commit 8d23db3 into ivoa-std:master Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

{links} response content-type header Outdated UCD for ObsCore obs_publisher_did
5 participants