-
Notifications
You must be signed in to change notification settings - Fork 178
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
Cleanup API spec #432
Cleanup API spec #432
Conversation
Hey @matthewhanson - just realized we have an approved PR for some API stuff - #415 I was about to merge, but then thought I should pause in case it might mess up this PR. |
I added some comments and have some more thing I wasn't able to add review comments for:
|
Cleaned up some of the intro Item/GeoJSON language Changed WFS3 to WFS 3 - that's how they talk about it and it shows up better in google searches Added a bit more about STAC + WFS 3
tweaked a few things, changed from WFS3 to WFS 3
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.
Haven't got to fully review everything yet. And did a couple direct commits to the branch, I hope that was ok. Feel free to rollback if not. I held off on major things there.
I do think we should go ahead and merge this soon, and then we can do additional changes as PR's and discuss that way. Since overall this is a big step in the right direction, and most changes requested are minor.
api-spec/README.md
Outdated
|
||
## API Fragments | ||
**Schemas:** The API is described by the OpenAPI specification documents in the *[openapi](openapi/)* folder. |
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.
I don't think we need to call this 'schemas', as they aren't really schemas in the sense of JSON or XML schemas. I think we can just say 'OpenAPI documents' or 'OpenAPI fragments', 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.
From https://swagger.io/docs/specification/about/ it seems like maybe the best term is 'OpenAPI description'.
| /stac/search | Items | GeoJSON FeatureCollection of Items found | | ||
``` | ||
|
||
The `stac/search` endpoint is similar to the `items` endpoint in WFS3 in that it accepts parameters for filtering, however it performs the filtering across all collections. The parameters accepted are the same as the Filter Parameters above, however the *[extensions](extensions/README.md)* also provide advanced querying parameters. |
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.
So I'd like to add a line here like: 'The stac
endpoint should function as a complete Catalog
representation of all the data contained in the STAC API - all Item
s should be linked in some way from that root through catalogs/collections, and as item
links from those catalogs. But then I realized that sat-api isn't doing that yet.
I feel like we should have an endpoint that stac browser can point at and navigate through down to the items in the same way you can with a static catalog. Though perhaps it makes sense to call that endpoint /browse or something, and then it'd be easier to not make it required?
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.
STAC Browser can now browse APIs because it can @mojodna added the ability to resolve links with rel=items as well as rel=item.
I actually removed the existing paragraph about the API sub-catalogs and opened an issue for it:
https://github.com/radiantearth/stac-spec/issues/438
So in short, yes we should discuss how a browsable API should work and can add a better description of it, but we can make it another PR.
@m-mohr Not sure what you mean here. The table of API extensions in extensions/README.md does link to each one. Or do you mean something else? |
Oh, and also didn't update Open API schema to something else yet, but will update that everywhere when we agree what it should be called. |
I meant the extension page with the content extensions (/extensions/README.md) should link to the API extensions (/api-spec/extensions/README.md). Regarding the other issue, I would just call the folder "openapi". In tests I usually use OpenAPI document for the full file, and OpenAPI fragments for parts. |
Ok thanks, I added a couple sentences in the extensions/ folder differentiating between content and API extensions and added link. Also removed "schema" from OpenAPI references. That takes care of all the comments so far, other than the Browsable API, which is handles by #438 |
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.
Looks good now, great work. Added some minor changes (URL fixed + dependency update).
Will merge now so that I can base my changes on this PR.
…came up when regenerating for #432.
Initial draft, still need to clean up and expand the documentation in README.
This PR: