Updating to stac pydantic 3 #627
Conversation
|
@rhysrevans3 looks like we had the same idea |
| ) | ||
| fields[k] = (v.outer_type_, body) | ||
| for k, field_info in model.model_fields.items(): | ||
| fields[k] = (field_info.annotation, field_info) |
There was a problem hiding this comment.
@thomas-maschler sorry I should have checked before putting in the pull request it looks like most of our changes are the same. I found the field_info.annotation would leave out the Optional type unless I switched from typing List and Dict to the inbuilt list and dict.
| @@ -2,6 +2,9 @@ | |||
|
|
|||
| ## [Unreleased] | |||
|
|
|||
| * Removing support for Python 3.8 | |||
There was a problem hiding this comment.
Why is this needed? pydantic 2 does support python 3.8 https://github.com/pydantic/pydantic/blob/main/pyproject.toml#L65
| @@ -1,4 +1,4 @@ | |||
| FROM python:3.8-slim as base | |||
| FROM python:3.9-slim as base | |||
There was a problem hiding this comment.
agree to remove python 3.8 but then we should use python 3.11 IMO
| extensions: List[ApiExtension] = attr.ib(default=attr.Factory(list)) | ||
| exceptions: Dict[Type[Exception], int] = attr.ib( | ||
| extensions: list[ApiExtension] = attr.ib(default=attr.Factory(list)) | ||
| exceptions: dict[Type[Exception], int] = attr.ib( |
There was a problem hiding this comment.
FYI is using dict directly instead of typing.Dict is the reason to switch to python>=3.9, typing.Dict is just an alias of dict (https://github.com/python/cpython/blob/3.9/Lib/typing.py#L1743C15-L1743C19) so it's fine to use typing.Dict IMO
https://stackoverflow.com/questions/37087457/difference-between-defining-typing-dict-and-dict
There was a problem hiding this comment.
@vincentsarago I found that during the create_request_model in models.py. The field_info.annotation would leave out the Optional part of the type. The only solution I found was to switch from typing List and Dict to the generic list and dict. There maybe a better solution to this?
|
Is there a reason to keep both this and #625 open? Based on a quick review, looks like #625 has less changes - and by the author of the stac-pydantic upgrade (thank you @thomas-maschler !). If we can close this and focus the review on #625 that will be easier on reviewers. @rhysrevans3 anything in here related to the pydantic upgrade that deem a must-have that #625 doesn't already contain? Otherwise consolidating on #625 and getting your review on it would be a good path to get this unblocked IMO. Thanks for this PR, unfortunate instance of duplication of work! |
Related Issue(s):
Description:
PR Checklist:
pre-commithooks pass locallymake test)make docs)