asset definition extension added #552
Conversation
|
@m-mohr I have resolved your comments |
|
Thanks, I hadn't finished the review this morning, finished it now with some additional ToDos. |
|
Made the changes requested, except I kept title as required, that's still open for discussion. |
m-mohr
left a comment
m-mohr
left a comment
There was a problem hiding this comment.
Thanks. There were still some additional problems in the JSON Schema and the README, which I just fixed for your convenience.
Will approve once we have a consensus on the (not?) required title. I still think it's a problem to require it here. A script that crawls Items and updated the Collection schemas automatically would fail to comply. It either doesn't have a title, leave the schema out or have to invent one. All three options are not good.
|
Ah cool, thanks for the fix, still figuring out this json-schema thing |
cholmes
left a comment
cholmes
left a comment
There was a problem hiding this comment.
Looks decent, but I think it needs more of the 'why', use cases and why implementors should make use of it.
| This extension introduces a single new field, `assets` at the top level of a collection. | ||
| An Asset Object defined at the Collection level is nearly the same as the [Asset Object in Items](../../item-spec/item-spec.md#asset-object), except for two differences. | ||
| The `href` field is not required, because collections don't point to any data by themselves. | ||
| Additioanlly the remaining fields, `title` and `type` are required in the Asset Definition, in order for it to adequately describe Item assets. |
There was a problem hiding this comment.
Minor type on 'Additionally'
|
|
||
| **Extension [Maturity Classification](../README.md#extension-maturity): Proposal** | ||
|
|
||
| A Collection extension to provide details about assets that are available in member Items. |
There was a problem hiding this comment.
I think we should include more 'why' - what are the use cases for an implementor to make use of this. Not sure if this is the exact right place. But a bit more here would be good, and maybe flesh out more of it down below.
m-mohr
left a comment
m-mohr
left a comment
There was a problem hiding this comment.
Fixed the example, otherwise looks good.
Related Issue(s): #153 #289
Proposed Changes:
PR Checklist:
npm run generate-allto update the generated OpenAPI files.