-
Notifications
You must be signed in to change notification settings - Fork 71
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: Avro dataclass introspect typing #976
fix: Avro dataclass introspect typing #976
Conversation
45215d3
to
f736125
Compare
f736125
to
e54ad60
Compare
@@ -128,7 +135,7 @@ def _field_type(field: Field, type_: object) -> AvroType: # pylint: disable=too | |||
T = TypeVar("T", str, int, bool, Enum, None) | |||
|
|||
|
|||
def transform_default(type_: type[T], default: T) -> str | int | bool | None: | |||
def transform_default(type_: type[T] | str, default: T) -> str | int | bool | 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.
What is the failure case that necessitates the addition of str
here? Just curious.
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.
This is pure typing correction. It looks that primitive types use string instead of type[T]
.
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.
Tracking this to here:
When originally authored, this type was type
, only changed recently and I guess propagated in most recent mypy bump:
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.
_field_type
doesn't give a type error because it accepts object
and makes no assumptions. It's probably somewhat likely to break when the new deferred annotations arrive, but I wouldn't deal with that until it's possible to test with.
Coverage reportClick to see where and how coverage changed
The report is truncated to 25 files out of 76. To see the full report, please visit the workflow summary page. This report was generated by python-coverage-comment-action |
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.
LGTM 👍
About this change - What it does
References: #xxxxx
Why this way