Skip to content

Fix deprecated UnicodeDelimitedTuple due to deprecated _field_types #35

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

Merged
merged 5 commits into from
Jul 13, 2021

Conversation

NazarioJL
Copy link
Contributor

@NazarioJL NazarioJL commented Jul 9, 2021

This addresses

The _field_types was removed in Python 3.9 causing UnicodeDelimitedTupleAttribute to not function properly.

This PR needs to be rebased onto #34

@NazarioJL NazarioJL force-pushed the fix_unicode_tuple branch from b6ca232 to 2b2e02a Compare July 9, 2021 17:39
@NazarioJL NazarioJL force-pushed the fix_unicode_tuple branch from 2b2e02a to 2ef5a62 Compare July 9, 2021 21:23
@NazarioJL
Copy link
Contributor Author

@ikonst 👀

NazarioJL added 2 commits July 9, 2021 20:03
…ted _field_types

 pick 4ad4f4b Fix UnicodeDelimitedTupleAttribute to use __annotations__ for deprecated _field_types
@NazarioJL NazarioJL force-pushed the fix_unicode_tuple branch from 2ef5a62 to a7d8b32 Compare July 10, 2021 03:12
@@ -68,3 +72,20 @@ def serialize(self, value: T) -> str:
f"Tuple elements may not contain delimiter '{self.delimiter}'",
)
return self.delimiter.join(strings)

def _parse_value(self, str_value: str, type_: Type[Any]) -> Any:
if hasattr(type_, "__args__"):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you trying to parse Union types here?

Copy link
Contributor Author

@NazarioJL NazarioJL Jul 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. This works for both, 3.8 and 3.9. In 3.9 Optional[T] types are used as Union[NoneType, T], but __args__ has the specific types in either. It will basically find the first parsable string and assign that value. There are inherent issues (that already existed) for example:

class AmbivalentTuple(NamedTuple):
    a_value: Union[str, int, float]
    b_value: Union[int, str, float]
    c_value: Union[float, int, str]

If the persisted record looks like "2::2::2" we will get a tuple that looks like:

AmbivalentTuple(a_value="2", b_value=2, c_value=2.0)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think __args__ exist in any generic type, not just Union, so at the very least I'd ensure the type is Union. But honestly I'd omit this. The user would get an TypeError: Cannot instantiate typing.Union error and that'll be enough to understand that the NamedTuple should consist of simple types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So leave as is?
In this case:

>>> typing.Union[int, str].__args__
(<class 'int'>, <class 'str'>)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, don't try to parse __args__. Simply t(value) and be done with it (like we did before).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, I think It would be nicer to do so, I'll go ahead and remove this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Constructs like typing.Union within the Python ecosystem are intended for type checkers to parse, not runtime. I doubt there's many counter-examples, perhaps in specialized libraries like pydantic? A DB ORM making any sense of them is probably an example of a library doing things you don't expect it to :)

One possibility could be to even do away with the option of parsing into an int. In that case, you can forego all this annotations inspection and define:

T = TypeVar("T", bound=Tuple[str, ...])

and only do return self.tuple_type(*values) in the deserializer.

However, this runs counter to the sample code. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ikonst Removed!

@NazarioJL NazarioJL force-pushed the fix_unicode_tuple branch from 36cdd49 to 4d2e62e Compare July 10, 2021 04:20
@ikonst ikonst merged commit 1fd641d into lyft:master Jul 13, 2021
@matthewpick
Copy link

Thanks @mathom for originally finding this issue during #31

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants