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

Add type hints to load_data #831

Merged
merged 5 commits into from Feb 10, 2017

Conversation

janga1997
Copy link
Member

@janga1997 janga1997 commented Jan 29, 2017

Fixes part of #808.

@drvinceknight
Copy link
Member

This looks fine to me, but worth @marcharper who wrote this part of the library taking a look :) 👍

@marcharper
Copy link
Member

Filenames are tricky, they can actually be bytes. We don't use bytes filenames in this library but a user could, and the type checker doesn't like it. I suggest Union[str, bytes].

@marcharper
Copy link
Member

Here's the mypy output:

axelrod/load_data_.py:9: error: Argument 1 to "join" of "str" has incompatible type "Tuple[str, Union[str, bytes]]"; expected Iterable[str]
axelrod/load_data_.py:11: error: Incompatible types in assignment (expression has type "str", variable has type "bytes")
axelrod/load_data_.py:13: error: Argument 1 to "split" of "bytes" has incompatible type "str"; expected "bytes"
axelrod/load_data_.py:14: error: Argument 1 to "startswith" of "bytes" has incompatible type "str"; expected "Union[bytes, Tuple[bytes, ...]]"
axelrod/load_data_.py:16: error: Argument 1 to "split" of "bytes" has incompatible type "str"; expected "bytes"
axelrod/load_data_.py:18: error: Incompatible return value type (got List[List[bytes]], expected List[List[str]])

Copy link
Member

@drvinceknight drvinceknight left a comment

Choose a reason for hiding this comment

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

Could you add this file to the type_tests.sh script please: https://github.com/Axelrod-Python/Axelrod/blob/master/type_tests.sh

@drvinceknight
Copy link
Member

Could you add this file to the type_tests.sh script please: https://github.com/Axelrod-Python/Axelrod/blob/master/type_tests.sh

Actually perhaps easiest to ignore that until #843 is merged. :)

@janga1997
Copy link
Member Author

@drvinceknight I am a little confused. Should I ignore this pull request for a while or should I not include this to the type_test.sh script?

@drvinceknight
Copy link
Member

@drvinceknight I am a little confused. Should I ignore this pull request for a while or should I not include this to the type_test.sh script?

Sorry, the confusion is my fault. Ignore my request for adding the module to type_test.sh.

I've run mypy locally on this branch and getting no errors so it looks fine to me but perhaps I'm missing something from @marcharper's comment.

@drvinceknight drvinceknight dismissed their stale review February 1, 2017 10:59

No need to add this module to type_test.sh for now.

@janga1997
Copy link
Member Author

janga1997 commented Feb 1, 2017

@drvinceknight Well, i am getting those errors too. As I pointed out in gitter, @marcharper wanted the option of bytes and str for filename. But the join function is facing problems as it expects a str iterable. I think it was decided to stick to str only. I am making the changes right now.

@drvinceknight
Copy link
Member

@drvinceknight Well, i am getting those errors too. As I pointed out in gitter, @marcharper wanted the option of bytes and str for filename. But the join and split functions are facing problems as they expect a str. I think it was decided to stick to str only. I am making the changes right now.

Ah yes! My mistake, I had run mypy incorrectly just then. That change sounds good :)

@janga1997
Copy link
Member Author

@drvinceknight The remaining errors also have something to do with str and bytes, but I can't figure out what.

@janga1997
Copy link
Member Author

@marcharper @drvinceknight The remaining errors that mypy produces:

axelrod/load_data_.py:9: error: Incompatible types in assignment (expression has type "str", variable has type "bytes")
axelrod/load_data_.py:11: error: Argument 1 to "split" of "bytes" has incompatible type "str"; expected "bytes"
axelrod/load_data_.py:12: error: Argument 1 to "startswith" of "bytes" has incompatible type "str"; expected "Union[bytes, Tuple[bytes, ...]]"
axelrod/load_data_.py:14: error: Argument 1 to "split" of "bytes" has incompatible type "str"; expected "bytes"
axelrod/load_data_.py:16: error: Incompatible return value type (got List[List[bytes]], expected List[List[str]])

These errors originate with the lines:

    data = pkg_resources.resource_string(__name__, path)
    data = data.decode('UTF-8', 'replace')

What is going on here?

@marcharper
Copy link
Member

It's a str/bytes thing with what pkg_resources returns, binary data instead of string data as you might expect. We could try casting to a string: data = str(data.decode('UTF-8', 'replace'))

@janga1997
Copy link
Member Author

janga1997 commented Feb 5, 2017

@marcharper I think you meant to say
data = str(pkg_resources.resource_string(__name__, path))
But I tried it, and it produces the error
axelrod/load_data_.py:9: error: "str" has no attribute "decode"; maybe "encode"? in the next line
data = data.decode('UTF-8', 'replace')

@marcharper
Copy link
Member

mypy is unhappy because we're overwriting a variable with a different type. This works:

    data_bytes = pkg_resources.resource_string(__name__, path)
    data = data_bytes.decode('UTF-8', 'replace')

@drvinceknight
Copy link
Member

Simple merge conflict here, in #852 I got rid of one of the functions in load_data.

@drvinceknight drvinceknight merged commit 6d136e1 into Axelrod-Python:master Feb 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants