Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Cordex extension and trying to build on higher level abstractions #64
base: master
Are you sure you want to change the base?
Cordex extension and trying to build on higher level abstractions #64
Changes from 17 commits
3c85233
f1c5683
c0bdeb2
3a4b6ef
b7d9b75
98c6420
33d78b9
0212008
04d94df
ac0cec2
03e5738
8cceeab
8b583bb
8db0847
df35431
c6e68e8
d76fdd8
adb1065
fd49161
08ccd42
2fb0eb6
312694c
a847c3f
72a8271
4817d66
2b43022
118810c
d1d838e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Could be improved with
requests
file-handler, allowing either local or remote URI, but not "blocking" for the PR.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 was unsure how to deal with references within a schema if it was not local.
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.
It's possible with the jsonschema library that we're using to have the library resolve remote references with
requests
. See the example here (which useshttpx
notrequests
but the idea is the same).I agree that this can be for another PR though
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.
Is there a reason these can't be a list of the extensions themselves? Why have this as a list of strings referring to class attributes?
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.
Well, I'm not sure then how we would pass the relevant attributes to each helper. I'll try to see if I can do something about this.
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 it is better to let the helpers add them automatically, as they might change over time (e.g.: new extension version with modified attributes).
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.
Sorry, I'll try to explain a bit better (or maybe I'm not understanding the issue).
Right now we do something like this:
Why can't we do this?
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.
Because of data ingestion.
For example, the THREDDS extension needs to be passed
THREDDSHelper(data['data']["access_urls"])
I currently manage this with
model_validator
:I'm not sure how we'd do that with your proposal without adding some obscure magic.
What I've now done is automatically detect extensions from the annotation (if it's a Helper subclass). Hope that's ok for now.
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 was actually misinterpreting the question thinking of
extensions
as theSTAC.Item.extensions
(ie: the URI to the applied extensions). I think we should rename the attribute, because that is very confusing. It should behelpers
to highlight the use of the helpers that have extended capabilities for applying the STAC extensions (and sometimes non-extension attributes).Ideally, we should have something like:
Because only classes of the helpers are used (not instances), they should be able to receive the item data dynamically for the
apply()
call.If the
data
source is needed, theHelper
base class could have it as a required argument forapply()
or in its__init__()
, whichever makes more sense.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.
Renamed
_extensions
to_helpers
.I don't see how I could implement your proposal, without hard-coding the
data
ingestion logic into the helpers themselves, which would couple them tightly with theLoader
, which I thought we should avoid.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.
Minor nitpick.
I think the
BaseSTAC
class should define something like:This way, any derived class and smart IDE flags right away that
helpers
must be overridden.I think
helpers
should be used instead of_helpers
because it is part of the "public" interface of that class, for anyone that derives a new implementation from it.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.
Should probably be a class method.
As a property, the name feels like an attribute to the current model. However, it seems to be employed as a "generate new UUID" helper method for each STAC Item created.
Also, import at the top.