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

Update collection ext #116

Merged
merged 2 commits into from
Jun 21, 2018
Merged

Update collection ext #116

merged 2 commits into from
Jun 21, 2018

Conversation

matthewhanson
Copy link
Collaborator

Updated collection extension to use a prefix in the field names it adds. Also adds a field so now there is:

cx:id
cx:name
cx:description

cc @cholmes @mojodna

Copy link
Contributor

@cholmes cholmes left a comment

Choose a reason for hiding this comment

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

Looks good. One point of clarification I think I need, a couple of suggestions:

  • Is the collection link required? It'd put some stronger language there.

  • Perhaps emphasize a bit more in the explanation that id + name are required, and explain a bit more about what they provide and how to choose them.

  • For name - is this for display? Perhaps spell that out - it's a human readable name for the collection, to be used in display of the record.

@cholmes
Copy link
Contributor

cholmes commented Jun 21, 2018

Oh, the other thing that would be really nice to have, though I'm not exactly sure how to make it, is a json schema validator. Like ideally the main validator would have a way to 'add on' an extension's json, so you could have it check your cx: or eo: fields too. But definitely not required to get this PR in.

@cholmes cholmes merged commit a0ea862 into dev Jun 21, 2018
@cholmes cholmes added this to the 0.5.0 milestone Jun 28, 2018
@mojodna
Copy link
Collaborator

mojodna commented Jun 29, 2018

I realized somewhat belatedly that there's no reason it needs to be a 2-letter prefix. c: would work just as well and be slightly less baroque.

@@ -63,7 +63,7 @@
"self": { "rel":"self", "href": "http://landsat-pds.s3.amazonaws.com/L8/153/025/LC81530252014153LGN00/LC81530252014153LGN00.json"},
"alternate": { "rel":"alternate", "href": "https://landsatonaws.com/L8/153/025/LC81530252014153LGN00", "type": "html"},
"catalog": { "rel":"catalog", "href": "http://landsat-pds.s3.amazonaws.com/L8/catalog.json"},
"collection": { "rel":"collection", "href": "http://landsat-pds.s3.amazonaws.com/L8/L1T-collection.json"}
"cx:collection": { "rel":"cx:ollection", "href": "http://landsat-pds.s3.amazonaws.com/L8/L1T-collection.json"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo: cx:ollection.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed in #122 (among other things)

@scisco scisco deleted the update_collection_ext branch August 16, 2018 00:00
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.

4 participants