fix: Update push_data and user_data annotation with JsonSerializable instead of Any#1889
fix: Update push_data and user_data annotation with JsonSerializable instead of Any#1889Mantisus wants to merge 3 commits into
push_data and user_data annotation with JsonSerializable instead of Any#1889Conversation
vdusek
left a comment
vdusek
left a comment
There was a problem hiding this comment.
Could you please try using this type:
JsonSerializable = dict[str, 'JsonSerializable'] | list['JsonSerializable'] | str | int | float | bool | None
"""Recursive type for JSON-serializable values - primitives plus objects and arrays with JSON-serializable contents.
Based on the definition discussed in https://github.com/python/typing/issues/182.
"""All major type checkers support recursive types now, so we can finally type this correctly. I recently made the same change in the API client as well - https://github.com/apify/apify-client-python/blob/master/src/apify_client/_types.py#L30C1-L34C4.
vdusek
left a comment
vdusek
left a comment
There was a problem hiding this comment.
The point of a better JsonSerializable type should be that it lets us type its various usages more accurately and with less complexity. Here are a few examples:
vdusek
left a comment
vdusek
left a comment
There was a problem hiding this comment.
Also see data field in the DatasetItemDb:
data: Mapped[list[dict[str, Any]] | dict[str, Any]] = mapped_column(JsonField, nullable=False)=>
data: Mapped[dict[str, JsonSerializable]] = mapped_column(JsonField, nullable=False)
| # Workaround for https://github.com/pydantic/pydantic/issues/9445 | ||
| J = TypeVar('J', bound='JsonSerializable') | ||
| JsonSerializable = list[J] | dict[str, J] | str | bool | int | float | None | ||
| JsonSerializable = dict[str, 'JsonSerializable'] | list['JsonSerializable'] | str | int | float | bool | None | ||
| else: | ||
| from pydantic import JsonValue as JsonSerializable |
There was a problem hiding this comment.
Just wondering whether now we can't just use Pydantic's type for type checking as well, have you tried it? 🙂
| yield {} | ||
|
|
||
| @staticmethod | ||
| def _is_list_of_items( |
There was a problem hiding this comment.
nit: maybe we can name it "is sequence"? instead of "is list" 🙂
| self._id = id or crypto_random_object_id(length=10) | ||
| self._max_age = max_age | ||
| self._user_data = user_data or {} | ||
| self._user_data: dict[str, JsonSerializable] = dict(user_data) if user_data is not None else {} |
There was a problem hiding this comment.
no?
| self._user_data: dict[str, JsonSerializable] = dict(user_data) if user_data is not None else {} | |
| self._user_data: MutableMapping[str, JsonSerializable] = dict(user_data) if user_data is not None else {} |
| id: Annotated[str, Field(alias='id')] | ||
| max_age: Annotated[timedelta, Field(alias='maxAge')] | ||
| user_data: Annotated[dict, Field(alias='userData')] | ||
| user_data: Annotated[dict[str, JsonSerializable], Field(alias='userData')] |
There was a problem hiding this comment.
Mapping / MutableMapping no?
| attribute: str = 'href', | ||
| label: str | None = None, | ||
| user_data: dict | None = None, | ||
| user_data: dict[str, JsonSerializable] | None = None, |
There was a problem hiding this comment.
Mapping / MutableMapping no?
| attribute: str | None = None, | ||
| label: str | None = None, | ||
| user_data: dict[str, Any] | None = None, | ||
| user_data: dict[str, JsonSerializable] | None = None, |
There was a problem hiding this comment.
Mapping / MutableMapping no?
| attribute: str = 'href', | ||
| label: str | None = None, | ||
| user_data: dict[str, Any] | None = None, | ||
| user_data: dict[str, JsonSerializable] | None = None, |
There was a problem hiding this comment.
Mapping / MutableMapping no?
3 participants
Description
Anywith explicitJsonSerializabletype forpush_dataanduser_dataparameters.Issues
push_dataannotations to useJsonSerializabletype #1191