-
Notifications
You must be signed in to change notification settings - Fork 47
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
Fragments #72
Conversation
I may do some more work on this, but marking as ready for review as it's a chunk of work that improves things. I'm thinking of just holding off on trying to do the ogcapi-features extensions for beta.1, since we didn't really do them before. I did just put the item-search extensions all in the item-search document, but I do think they should evolve to be each in their own file or possibly even folder. I do think we should get to the testable assertions style that OGC has (though those should be the last thing people read, instead of the main spec narrative), and then it'd make sense to have each of those in an extension. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work. I added some comments and suggestions, but nothing major.
fragments/query/README.md
Outdated
|
||
The syntax for the `query` filter is: | ||
|
||
```js | ||
```json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommend to revert this change as the example is invalid JSON and JS renders this a bit better (i.e. I chose JS specifically)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, gotcha - didn't realize it was deliberate.
@@ -75,6 +49,8 @@ GET /search?collections=landsat8,sentinel&bbox=-10.415,36.066,3.779,44.213&limit | |||
} | |||
``` | |||
|
|||
For more examples see [examples.md](examples.md). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also put the link into the "intro" list, here people may miss it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the 'intro' list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The list with maturity, conformance uri etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line 3-6 in the file
item-search/README.md
Outdated
POST /search | ||
Search-After: LC81530752019135LGN00 | ||
``` | ||
STAC API's that support the query functionality must include the conformance class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conformsTo details are repeated many times. Maybe there a more clever way without writing so much? The shorter the docs, the more likely people read it. Is it enough to specify Conformance URI at the top and have somewhere centrally defined that you need conformance classes? At a place you can't miss it. A bold note in the core or so...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that's fair. I think for these I was going more towards the 'requirements' style, to make clear exactly what you have to do. But my plan is to do actual requirements in a next release, in their own document, and to keep this one as the narrative. So I think it's ok to just remove some of the detail/repetition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need to document conformsTo on the landing page anyway, we have not done that yet: #27 (comment)
Co-authored-by: Matthias Mohr <[email protected]>
Co-authored-by: Matthias Mohr <[email protected]>
We also need to discuss extensions and Conformance URIs a bit more (call on Monday?), e.g. currently there's a # in the item-search extensions as the don't have a separate OpenAPI and folder. It's different for ogcapi-features extensions, we should think about a guideline maybe how to structure new extensions in the future. |
Sounds good on discussing on monday. I do think it'll probably make sense to put the item-search extensions in folders eventually, with readme, examples and 'requirements' - but they won't have an openapi yaml, since it's supplied by the fragment. I addressed most of the comments, so will merge this in. The one I didn't understand was on the examples:
Not sure which intro list you're talking about. |
@cholmes line 3 - 6 in the file. The introductory list that has the important details, could as last item has a link to examples. |
Got it. That still took me a minute - I kept interpreting it as add a link at that line to the list at the top. Adding a link to examples directly from the top makes way more sense. |
Oh, yeah, sorry, sometimes one is on the wrong track and then it's hard to get them back on the right track ;-) Can be merged! |
Related Issue(s): #
Proposed Changes:
PR Checklist:
npm run generate-all
to update the generated OpenAPI files.