-
Notifications
You must be signed in to change notification settings - Fork 122
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
Default layout strategy for catalogs #1295
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1295 +/- ##
==========================================
+ Coverage 91.12% 91.15% +0.03%
==========================================
Files 51 51
Lines 6994 7002 +8
Branches 1001 1002 +1
==========================================
+ Hits 6373 6383 +10
Misses 444 444
+ Partials 177 175 -2 ☔ View full report in Codecov by Sentry. |
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.
This looks really good too! The only issue is consolidating on an argument name.
docs/concepts.rst
Outdated
|
||
catalog = Catalog(..., | ||
href="/some/location/catalog.json", | ||
layout_strategy=custom_strategy) |
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.
In other places we just call this kwarg strategy
- did that feel too opaque?
pystac/catalog.py
Outdated
@@ -167,6 +171,9 @@ class Catalog(STACObject): | |||
a canonical format. | |||
""" | |||
|
|||
_layout_strategy: HrefLayoutStrategy = BestPracticesLayoutStrategy() |
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 the idea is that pystac will be able to override this in the Client
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.
Yes, correct. For example, the pystac_client.Client
could set the fallback strategy to APILayoutStrategy
Thanks for the review, I updated the argument name. |
if root.strategy is not None: | ||
return root.strategy | ||
else: | ||
return root._fallback_strategy |
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 rename is a lot clearer too 👍
Related Issue(s):
Description:
This PR enables setting a default layout strategy for a catalog. Users can either set the default layout strategy
when instantiating the catalog or defining a fall back strategy when subclassing Catalog.
Together with #1294, this PR is part of setting the foundation for enabling API transactions using
pystac-client
.PR Checklist:
pre-commit
hooks pass locallyscripts/test
)