Updating models #39
Updating models #39
Conversation
There was a problem hiding this comment.
Thanks for making these changes @sturoscy-personal they look great. One general comment do we have access to the vocab yet to check the typing/which properties need to be lists? These types look good to me if we don't have that yet. 👍
esgf_playground_utils/models/item.py
Outdated
|
|
||
|
|
||
| class CMIP6Item(Item): | ||
| properties: CMIP6ItemProperties |
There was a problem hiding this comment.
Should this be ESGFItemProperties or should the class about be CMIP6ItemProperties?
There was a problem hiding this comment.
ESGFItemProperies makes more sense :)
esgf_playground_utils/models/item.py
Outdated
| data_specs_version: str | ||
| citation_url: HttpUrl | ||
| data_spec_version: Optional[str] = None | ||
| datetime: Optional[datetimevalidate.datetime] = None |
There was a problem hiding this comment.
datetime, start_datetime and end_datetime are included in StacCommonMetadata which ItemProperties inherits from. So could be left out of this model unless datetimevalidate gives better validation?
There was a problem hiding this comment.
Probably doesn't and would rather use built-in types. I can make the switch.
| @@ -20,14 +20,15 @@ class _Payload(BaseModel): | |||
|
|
|||
| """ | |||
|
|
|||
| collection_id: str | |||
| key: str | |||
There was a problem hiding this comment.
Sorry I think I've forgotten what key was?
There was a problem hiding this comment.
key is the collection_id, but maybe it makes more sense to just use collection_id then? We had it as key for some reason.
|
|
||
|
|
||
| class CreatePayload(_Payload): | ||
| """ | ||
| Model describing a ``CREATE`` payload. This must be sent as a ``POST`` request. | ||
| """ | ||
|
|
||
| collection_id: str |
There was a problem hiding this comment.
I think the collection_id will need to be added to UpdatePayload, RevokePayload, PartialUpdatePayload (don't think they're in use yet) if it's being removed from the base _Payload class. @djspstfc wrote these models so probably has a stronger opinion.
There was a problem hiding this comment.
I think its fine to use collection_id instead of key. Makes the base _Payload work again.
esgf_playground_utils/models/item.py
Outdated
| product: str | ||
| project: str | ||
| realm: List[str] | ||
| retracted: Optional[str] = None |
There was a problem hiding this comment.
Should this be a boolean? Or can it take other values than true or false
There was a problem hiding this comment.
If you mean retracted, I think you're right, we were going to set that to True or False.
There was a problem hiding this comment.
Sorry yes I did mean retracted 👍
|
As the models have changed, can we update the version number in the pyproject.toml file (to 0.5.0)? Thanks! |
General:
Quality checks:
versionandauthorattributes (as appropriate) in the pyproject.toml file?Community standards:
What is the purpose of this change?
The purpose of this change is to...
What does the change consist of?
This change consists of...