-
Notifications
You must be signed in to change notification settings - Fork 157
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
Added static typing to data_readers/avro_data.py #657
Added static typing to data_readers/avro_data.py #657
Conversation
@@ -11,9 +13,9 @@ | |||
class AVROData(JSONData, BaseData): | |||
"""AVROData class to save and load spreadsheet data.""" | |||
|
|||
data_type = "avro" | |||
data_type: Optional[str] = "avro" |
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.
wouldn't this just be str
not Optional[str]
... not sure we would want to allow for None
https://mypy.readthedocs.io/en/stable/kinds_of_types.html#optional-types-and-the-none-type
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.
@taylorfturner I wonder if it has to do with the Abstract being None
until defined. But agreed, if it can be str to be str
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.
When I annotate with str
, mypy complains because it's set to None
in BaseData
.
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.
Yup, makes sense.
@@ -74,18 +81,20 @@ def is_match(cls, file_path, options=None): | |||
|
|||
# get current position of stream | |||
if data_utils.is_stream_buffer(file_path): | |||
assert not isinstance(file_path, str) |
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.
is this required by mypy
?
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's needed for mypy but I now changed it into an extra condition in the if statement instead of an assertion.
starting_location = file_path.tell() | ||
|
||
is_valid_avro = fastavro.is_avro(file_path) | ||
|
||
# return to original position in stream | ||
if data_utils.is_stream_buffer(file_path): | ||
assert not isinstance(file_path, str) |
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.
is this required by mypy
?
@@ -125,13 +132,13 @@ def _get_nested_keys_from_dicts(cls, dicts): | |||
:type dicts: list(dict) | |||
:return: a dictionary containing nested keys | |||
""" | |||
nested_keys = {} | |||
nested_keys: Dict |
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.
I think this still needs to be instantiated with = {}
#610