-
Notifications
You must be signed in to change notification settings - Fork 14
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add query-tables method #32
Conversation
@@ -41,7 +42,7 @@ def _handle_http_status(response: Response) -> Optional[Response]: | |||
@dumps.to_json(JsonModel) | |||
def _deserialize_model(model_cls: Type[JsonModel], model_instance: JsonModel) -> Dict: | |||
"""Turns a :class:`.JsonModel` instance into a dictionary for serialization.""" | |||
return model_instance.dict(by_alias=True, exclude_unset=True) | |||
return json.loads(model_instance.json(by_alias=True, exclude_unset=True)) |
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.
Working around this issue: pydantic/pydantic#1409
tl;dr: calling dict()
on a model doesn't serialize datetimes like json()
does, but the converter is expecting a dictionary.
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 could be nice to have tests for integer and boolean filter substitutions too. Such as supportsAppend == @0 and rowCount < @1
between now and the start of the current year | ||
""" | ||
|
||
substitutions: Optional[List[Union[StrictInt, StrictBool, str, None]]] = None |
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 could be interesting to explore having the API accept datetime (and maybe something representing a TimeSpan) values and serializing them to strings in the correct format.
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 client-side substitutions for Python objects? That sounds useful - I'll keep it in mind.
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.
Yep. Could be worth adding a task/story. Similar to how we want to support a helper for reading and writing row data as native types instead of only strings.
I added more filters to the test. |
What does this Pull Request accomplish?
Why should this Pull Request be merged?
What testing has been done?