-
Notifications
You must be signed in to change notification settings - Fork 102
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
re-formatting collections endpoint to match spec. #232
Conversation
Also closes #108, it appears |
If you want to add me as a contributor on your fork, @rsmith013, I'd be happy to lend a hand |
@moradology done |
I think I've nailed down the implementation bits and the most relevant tests but there's one remaining failure that continues to confuse. I will continue poking at it |
@rsmith013 Would you be alright with me forcing up a rebase to resolve conflicts with |
landing_page["links"].append( | ||
{ | ||
"rel": Relations.child.value, | ||
"type": MimeTypes.json.value, | ||
"title": collection.get("title"), | ||
"title": collection.get("title") or collection.get("id"), |
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 like this change is the culprit on the remaining test
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 should add: this change looks reasonable to me - do we know whether the test encodes something important about the STAC spec?
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.
@moradology I don't think the spec has an opinion. At least, not one which I have found. I was working under the assumption it was a standard URL so the title is for display rather than anything else. As you say, seemed reasonable.
@moradology go for it. I have kept my master branch clean and in line with the upstream. (At least, that was my goal.) |
a536d60
to
c1b2a28
Compare
@lossyrob Perhaps ready for for a once over and merge when you're able |
self.collection_serializer.db_to_stac(collection, base_url=base_url) | ||
for collection in collections | ||
] | ||
return response | ||
collection_list = Collections( | ||
collections=serialized_collections or [], links={} |
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.
Links should be a list
@@ -40,7 +40,8 @@ async def all_collections(self, **kwargs) -> List[Collection]: | |||
collection_id=coll["id"], request=request | |||
).get_links() | |||
linked_collections.append(coll) | |||
return linked_collections | |||
collection_list = Collections(collections=collections or [], links={}) |
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.
Links should be a 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.
Good catch. Which reminds me that we probably need something here rather than simply having the placeholders I added...
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 believe this is now handled correctly. Here's the example JSON:
{
"collections":[
{
"type":"Collection",
"id":"joplin",
"stac_extensions":[
],
"stac_version":"1.0.0",
"title":null,
"description":"This imagery was acquired by the NOAA Remote Sensing Division to support NOAA national security and emergency response requirements. In addition, it will be used for ongoing research efforts for testing and developing standards for airborne digital imagery. Individual images have been combined into a larger mosaic and tiled for distribution. The approximate ground sample distance (GSD) for each pixel is 35 cm (1.14 feet).",
"keywords":null,
"license":"public-domain",
"providers":null,
"summaries":null,
"extent":{
"spatial":{
"bbox":[
[
-94.6911621,
37.0332547,
-94.402771,
37.1077651
]
]
},
"temporal":{
"interval":[
[
"2000-02-01T00:00:00Z",
"2000-02-12T00:00:00Z"
]
]
}
},
"links":[
{
"rel":"self",
"type":"application/json",
"href":"http://localhost:8081/collections/joplin"
},
{
"rel":"parent",
"type":"application/json",
"href":"http://localhost:8081/"
},
{
"rel":"items",
"type":"application/geo+json",
"href":"http://localhost:8081/collections/joplin/items"
},
{
"rel":"root",
"type":"application/json",
"href":"http://localhost:8081/"
}
]
}
],
"links":[
{
"rel":"root",
"type":"application/json",
"href":"http://localhost:8081/"
},
{
"rel":"parent",
"type":"application/json",
"href":"http://localhost:8081/"
},
{
"rel":"self",
"type":"application/json",
"href":"http://localhost:8081/collections"
}
]
}
@@ -24,6 +27,7 @@ class CoreCrudClient(AsyncBaseCoreClient): | |||
async def all_collections(self, **kwargs) -> List[Collection]: |
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 think, this needs to be Collections
instead of List[Collection]
as type returned.
closes #231