Fix from_dict() in the presence of optional datetime fields. #329
Conversation
The added test fails without the fix to `__init__.py`: ``` > setattr(self, field_name, cls().from_dict(value[key])) E TypeError: function missing required argument 'year' (pos 1) src/betterproto/__init__.py:1204: TypeError ```
| @@ -1191,16 +1191,15 @@ def from_dict(self: T, value: Dict[str, Any]) -> T: | |||
| ] | |||
| else: | |||
| v = [cls().from_dict(item) for item in value[key]] | |||
| elif isinstance(v, datetime): | |||
| elif cls == datetime: | |||
There was a problem hiding this comment.
Can you change these to if cls is datetime?
There was a problem hiding this comment.
For whatever reason, there are a lot of instances of cls == foo in this file and none of cls is foo. I think it makes sense to maintain consistency but I'm happy to change all of them to use is if you prefer.
There was a problem hiding this comment.
I agree with both of you, and it was courteous of you to match the existing style, @emosenkis . I'd say given that it's a style point, leave as-is and fix on a refactor.
There was a problem hiding this comment.
Thanks @emosenkis for the test and fix!
Before we merge - please create an issue describing the problem (basically the same as the MR description) and link this MR to it. That will help other people discover both the bug and the fix when they run into it.
| @@ -1191,16 +1191,15 @@ def from_dict(self: T, value: Dict[str, Any]) -> T: | |||
| ] | |||
| else: | |||
| v = [cls().from_dict(item) for item in value[key]] | |||
| elif isinstance(v, datetime): | |||
| elif cls == datetime: | |||
There was a problem hiding this comment.
I agree with both of you, and it was courteous of you to match the existing style, @emosenkis . I'd say given that it's a style point, leave as-is and fix on a refactor.
|
Thanks! Could you push to pypi? |
Fixes #333
The added test fails without the fix to
__init__.py: