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

for templates the format must be "uri-template". #110

Merged
merged 2 commits into from
Oct 8, 2024

Conversation

redmitry
Copy link
Collaborator

@redmitry redmitry commented Sep 5, 2023

in the beaconMapSchema.json some URLs are templates and must have the uri-template" format (#103).

@redmitry redmitry requested a review from mbaudis September 5, 2023 17:30
@jrambla
Copy link
Contributor

jrambla commented Jan 9, 2024

IMU not all the uri you have modified are uri-templates (e.g. filteringterms).
I'm fine with changing all (?) to uri-templates, if a non-template uri could be expressed as uri-template,
is that true?

@costero-e
Copy link
Collaborator

I would say uri-template is more like a type of uri, not the other way round. If we say something is a uri-template we are excluding uri that don't include braces {} with a parameter.
What do you think @redmitry ?

@redmitry
Copy link
Collaborator Author

https://datatracker.ietf.org/doc/html/rfc6570

uri-template is an URI "with variables" - it is as in the name - a template to produce the URI.
Technically any URI should be a valid template with no variables...
My point is that for the endpoints that includes {xxx} variables are not valid URIs, but are "uri-template"s.

Another thing is that Json Schemas 2019-2020 by default should just ignore "format". My validator validates it and reports an error. Current Beacon Network uses "fixed" schema (not from github) for the /map:
see the difference: beaconMapResponse.json vs beaconEntryTypesResponse.json

So could we use "uri-template" in all places? Yes. But I see no point.

Best,

Dmitry

@costero-e
Copy link
Collaborator

Thank you @redmitry . So, are we still willing to accept this PR? Or can we close it?

@redmitry
Copy link
Collaborator Author

Thank you @redmitry . So, are we still willing to accept this PR? Or can we close it?

IMO this should be in the main branch.

Copy link
Collaborator

@costero-e costero-e left a comment

Choose a reason for hiding this comment

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

If uri-template is a valid uri even if it doesn't carry variables within braces, this can be accepted from my side.

@costero-e
Copy link
Collaborator

Still, one review pending, @mbaudis or @jrambla. Thank you.

@jrambla jrambla added this to the 2.2.0 milestone Oct 8, 2024
@jrambla jrambla self-requested a review October 8, 2024 05:15
@costero-e costero-e merged commit 5ea17fb into ga4gh-beacon:main Oct 8, 2024
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