-
Notifications
You must be signed in to change notification settings - Fork 1
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
Change include/exclude recommendations #5
Change include/exclude recommendations #5
Conversation
6f95b04
to
a409945
Compare
a409945
to
5e3a7f2
Compare
ea43541
to
db7d816
Compare
@philvarner re-requesting review, I've updated all the language to use "should", and to avoid "add" and "subtract" in favor of "included" and "exclude". |
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.
apparently I didn't submit these comments? I guess I accidentally started a review and they were all added to that. I'll delete the ones that aren't relevant.
2949fcb
to
0280530
Compare
At the risk of getting overly conceptual here, it might be useful to state that there are multiple levels of "modeling" of data in this. The image data is a model of the real world, and likewise the STAC Item is a model of the image data. Fields is sometimes causing the STAC Item metadata to be manipulated in a way that means it's not a exact copy of what's stored in the database, but a model of that. So, for example, if we choose to return a geometry=null and bbox value to return a valid STAC Item when geometry is excluded, we're modeling the metadata to conform to the fields restrictions. |
I guess? To me, it's simpler than that -- if someone says they don't want it, don't give it to them. IMO we can trust the client to know what it wants (i.e. a invalid STAC item). |
Yeah, that's probably not something the server should try to do, since the validity guardrails are off when you start using fields. |
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.
comments inline. I think we're close to having this done.
e39f0bb
to
3df4025
Compare
@philvarner sorry I squashed and force pushed and the repo requires a re-review, didn't realize that. |
3df4025
to
f0c062b
Compare
Related Issue(s):
includes
stac-utils/stac-fastapi-pgstac#27Proposed Changes:
include
semantics to only return what's asked for. Previously, the recommendation was to return what was asked for AND the default fields.stac_version
in the default fields.PR Checklist: