Repository navigation
Conversation
| yield DataField(name=name, value=item) | ||
| for name, data_value in data.items(): | ||
| if data_value is None or isinstance( | ||
| data_value, (*PRIMITIVE_DATA_TYPES, bytes) |
There was a problem hiding this comment.
DataField accepts bytes in addition to the other "primitive types". I'm not sure where else PrimitiveTypes is being used but we might want to consider expanding everywhere to accept bytes instead of just here.
| for name, file_value in file_items: | ||
| yield FileField(name=name, value=file_value) |
There was a problem hiding this comment.
mypy complains about sharing the variable value for both data and file types
| if isinstance(value, (list, tuple)): | ||
| plain_data.extend([(key, primitive_value_to_str(item)) for item in value]) | ||
| else: | ||
| if value is None or isinstance(value, PRIMITIVE_DATA_TYPES): | ||
| plain_data.append((key, primitive_value_to_str(value))) | ||
| else: | ||
| plain_data.extend([(key, primitive_value_to_str(item)) for item in value]) |
There was a problem hiding this comment.
Flipping these checks lets us handle any sequence instead of special casing List and Tuple.
|
@tomchristie how would you feel about running pyright in CI so we can use it for informational purposes, maybe using it’s typing score as a rough metric for “is this PR improving things for users” without any sort of enforcement? |
|
I'd suggest that if we're going to start making type changes like this and #2266, then we do so with a more precise approach. Removing
|
| ): | ||
| yield DataField(name=name, value=data_value) | ||
| elif isinstance(data_value, typing.Sequence): | ||
| for item in data_value: |
There was a problem hiding this comment.
Moving the type checks to this function lets us handle any sequence
|
@tomchristie I requested re-review since I removed the removal (so restored?) the runtime type checks. This is still touching multiple modules but it is dealing only with a single type ( I think that the lack of changes to the tests (besides adding a check for a case that would have previously been a false positive) are a good indication that this is not changing any runtime behavior despite the internal refactoring. |
|
Closing as stale |
Context: https://github.com/orgs/encode/teams/maintainers/discussions/2/comments/15
#2266