-
Notifications
You must be signed in to change notification settings - Fork 25
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
fix: pydantic v2 Warnings: The dict method is deprecated; use model_dump instead #49
fix: pydantic v2 Warnings: The dict method is deprecated; use model_dump instead #49
Conversation
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.
Hi @bVdCreations, my apologies for not seeing your PR. Please check my comment.
fastapi_events/dispatcher.py
Outdated
@@ -116,7 +118,10 @@ def dispatch( | |||
payload_schema_cls = payload_schema_registry.get(event_name) | |||
if payload_schema_cls: | |||
payload_schema_cls_dict_args = payload_schema_cls_dict_args or DEFAULT_PAYLOAD_SCHEMA_CLS_DICT_ARGS | |||
payload = payload_schema_cls(**(payload or {})).dict(**payload_schema_cls_dict_args) | |||
if IS_PYDANTIC_V2: |
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.
My sincerest apologies as I completely missed your contribution.
Here's my thought on this:
As model_dump(...)
is most likely to remain in future versions. It will be more robust if we inverse the if-else logic. I.e.: check if pydantic v1 is used before using .dict(...)
, and resort to model_dump(...)
in the else clause.
Once again, my apologies for not seeing your contribution earlier.
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.
@melvinkcx changed as requested
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.
Thanks, could you please pull the latest changes from the upstream? There has been a new release since your first change.
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.
@melvinkcx rebased and squashed the commits
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.
thank you very much!
dc98226
to
a481258
Compare
fixes issue #48