Skip to content
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 type checking issue with to_dict and to_ordered_dict #151

Merged

Conversation

pipermerriam
Copy link
Member

What was wrong?

The to_dict and to_ordered_dict had incorrect type hints.

Also updated to new mypy version that has some nice improvements

How was it fixed?

Added a test for each of the to_thing decorators to validate they do indeed preserve type data correctly.

Cute Animal Picture

maxresdefault

@pipermerriam pipermerriam requested a review from carver April 17, 2019 16:26
Copy link
Collaborator

@carver carver left a comment

Choose a reason for hiding this comment

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

👍 after figuring out what's up with the type tests

@@ -63,10 +63,10 @@ def inner(*args, **kwargs) -> T: # type: ignore
) # type: Callable[[Callable[..., Iterable[TVal]]], Callable[..., Set[TVal]]] # noqa: E501
to_dict = apply_to_return_value(
dict
) # type: Callable[[Callable[..., Iterable[Union[Mapping[TVal, TKey], Tuple[TVal, TKey]]]]], Callable[..., Dict[TKey, TVal]]] # noqa: E501
) # type: Callable[[Callable[..., Iterable[Union[Mapping[TKey, TVal], Tuple[TKey, TVal]]]]], Callable[..., Dict[TKey, TVal]]] # noqa: E501
Copy link
Collaborator

Choose a reason for hiding this comment

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

lol whoops

@@ -9,7 +9,7 @@
'lint': [
'black>=18.6b4,<19',
'flake8>=3.5.0,<4.0.0',
'mypy<0.600',
'mypy<0.701',
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice

@@ -34,7 +34,7 @@ def to_hex(
)

if is_integer(primitive):
return HexStr(hex(primitive))
return HexStr(hex(primitive)) # type: ignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thoughts on a type-cast instead of an ignore?

HexStr(hex(
  cast(int, primitive)
))

yield 3


v_reversed_return: Tuple[int] = typing_reversed_return()
Copy link
Collaborator

Choose a reason for hiding this comment

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

How did this pass the mypy test? Shouldn't it be Tuple[int, ...]?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is the same issue as #152 . I tried a number of things and couldn't get it to enforce the types so I dropped the tests for flatten/sorted/reversed and verified that the other tests are indeed enforcing the type. There aren't strong tests since evidently there are ways for that type of thing to silently fail, but they seem better than nothing. Right now it isn't clear how to do asserting that types are not valid easily so I'm inclined to move this forward and come back to that (opening an issue for it).

yield 3


v_sorted_return: Tuple[int] = typing_sorted_return()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here

@pipermerriam pipermerriam force-pushed the piper/fix-typing-issue-with-to-dict branch from fd61c63 to b9be70a Compare April 17, 2019 17:53
@pipermerriam pipermerriam merged commit 547337a into ethereum:master Apr 17, 2019
@pipermerriam pipermerriam deleted the piper/fix-typing-issue-with-to-dict branch April 17, 2019 17:54
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.

2 participants