Skip to content

Conversation

@ecederstrand
Copy link
Contributor

Refs #21

Alters the Base64IO.read() method to support string streams as well as bytes streams.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.


_data_buffer = io.BytesIO()
_data_buffer.write(b"".join(data.split()))
_data_buffer = io.BytesIO() if isinstance(data, bytes) else io.StringIO()
Copy link
Member

Choose a reason for hiding this comment

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

Method return type annotation needs to change to AnyStr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. The mypy tests are failing but I'm not sure why - I haven't used mypy previously. It looks like a false positive - maybe mypy is not able to handle the branching logic that handles bytes and str differently?

Copy link
Member

Choose a reason for hiding this comment

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

The initial problem is just that you need to add an import for AnyStr in the types imports at the top. I'm not sure if the other issues it's running into are just a side effect of missing that or not.

@ecederstrand
Copy link
Contributor Author

ecederstrand commented Nov 2, 2018

I have attempted to fix the mypy test but failed. Adding import of AnyStr helped, but mypy is not happy that _data_buffer could be either a StringIO or BytesIO. I don't know how to fix that.

Copy link
Member

@mattsb42-aws mattsb42-aws left a comment

Choose a reason for hiding this comment

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

I'm going to hack on this a bit before we publish both to see if I can sort out the type annotations and because I have a feeling it might need a couple tweaks to correctly handle unicode in both Python 2 and 3. I want to expand the tests to figure out whether that is an issue, but I don't think it's useful to try and iterate over that through this PR.

I think this is gets us most of the way there, thanks!

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