Skip to content
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

Enable conformance class configuration #214

Merged

Conversation

moradology
Copy link
Collaborator

This PR adds a conformance_classes parameter to the LandingPageMixin and all clients downstream from BaseCoreClient or AsyncCoreClient. This feature was added in a prior commit but appears to have been lost in the shuffle - a test has been added to hopefully avoid the regression going forward.

Closes #169

@moradology moradology changed the title Fix/conformance configurability Enable conformance class configuration Aug 10, 2021
@philvarner
Copy link
Collaborator

I just ran this against the STAC API Validator and is passed the conformance part of the tests (though it blows up because the /collections endpoint is wrong)

Copy link
Collaborator

@geospatial-jeff geospatial-jeff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! There will be some merge conflicts due to #199

@rsmith013
Copy link

@moradology In the filter extension PR, I added a method list_conformance_classes to generate the list of conformance classes based on which extensions were enabled by the server. I think it would be good to move this PR in that direction rather than adding an extra instance attribute to the core client which needs to be configured. Thoughts?

@rsmith013
Copy link

nice!

@moradology
Copy link
Collaborator Author

@rsmith013 I like the idea of separating base conformance classes from those which are clearly extensions but am not entirely sold on not making the base classes configurable (I could be convinced but wonder how many not-fully-compliant STACs are out there). I've added the behavior from your PR though, as it looks like a win

@lossyrob
Copy link
Member

What is the merge strategy here with #165? Should this be updated and merged, and then #165 updated with these changes, or do the changes not touch each other?

@rsmith013
Copy link

@lossyrob I would merge this first, then I expect there would be a minor conflict with #165 but should be easy enough to resolve.

@lossyrob
Copy link
Member

Sounds good - @moradology can you update to latest and then I'll merge?

@moradology moradology force-pushed the fix/conformance_configurability branch 2 times, most recently from 1c86dd5 to 745c111 Compare August 13, 2021 13:19
@moradology moradology force-pushed the fix/conformance_configurability branch from 745c111 to d0939bc Compare August 13, 2021 13:19
@moradology moradology force-pushed the fix/conformance_configurability branch from 98dd362 to 353935e Compare August 13, 2021 15:05
@moradology
Copy link
Collaborator Author

@lossyrob Pinging you so that you can push the button. Should be good to go

@lossyrob lossyrob merged commit faaec6f into stac-utils:master Aug 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API Landing Page not advertise conformance classes correctly
5 participants