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

Prefer io.BytesIO over six; available on all supported Pythons #45388

Merged
merged 1 commit into from
Sep 10, 2018
Merged

Prefer io.BytesIO over six; available on all supported Pythons #45388

merged 1 commit into from
Sep 10, 2018

Conversation

jdufresne
Copy link
Contributor

On all supported Pythons, the io.BytesIO is always a stream implementation using an in-memory bytes buffer. Makes code slightly more forward compatible by reducing use of the six module.

On all supported Pythons, the io.BytesIO is always a stream
implementation using an in-memory bytes buffer. Makes code slightly more
forward compatible by reducing use of the six module.
@ansibot
Copy link
Contributor

ansibot commented Sep 8, 2018

@jdufresne Greetings! Thanks for taking the time to open this pullrequest. In order for the community to handle your pullrequest effectively, we need a bit more information.

Here are the items we could not find in your description:

  • issue type

Please set the description of this pullrequest with this template:
https://raw.githubusercontent.com/ansible/ansible/devel/.github/PULL_REQUEST_TEMPLATE.md

click here for bot help

@ansibot
Copy link
Contributor

ansibot commented Sep 8, 2018

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 needs_info This issue requires further information. Please answer any outstanding questions. needs_template This issue/PR has an incomplete description. Please fill in the proposed template correctly. needs_triage Needs a first human triage before being processed. networking Network category small_patch support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests. labels Sep 8, 2018
@bcoca bcoca removed the needs_triage Needs a first human triage before being processed. label Sep 10, 2018
Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

LGTM

@webknjaz
Copy link
Member

@abadger
Copy link
Contributor

abadger commented Sep 10, 2018

Note: io.BytesIO and six.BytesIO are different. Not sure yet if that's going to make a difference. Looking into it.

@abadger
Copy link
Contributor

abadger commented Sep 10, 2018

>>> import io
>>> io.BytesIO
<class 'io.BytesIO'>
>>> import six
>>> six.BytesIO
<class StringIO.StringIO at 0x7f4811b87d70>
>>> io.BytesIO(u'test').read()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: expecting a bytes object, got unicode
>>> six.BytesIO(u'test').read()
u'test'

However, on Python3, they should be the same (and both traceback when given a non-bytes string). We'll have to check that there's nothing here that is only used on python2 before switching.

@abadger
Copy link
Contributor

abadger commented Sep 10, 2018

Networking team needs to review this for my above question and then can merge it.

A quick look and I think:

  • httpapi.py is okay to apply this patch
  • test/units/plugins/httpapi/test_ftd.py is okay but probably should be rewritten:
- response_data = BytesIO(response_text.encode() if response_text else ''.encode())
+ response_data = BytesIO(to_bytes(response_text, errors='strict') if response_text else b'')
  • network_cli.py: I'm not sure about this one. I think it hinges on what self._ssh_shell.recv(256) returns on both Python2 and Python3. If it always returns bytes (str type on python2 and bytes type on python3) then this is fine. if it sometimes returns bytes and sometimes returns text (unicode type on python2 or str type on python3) then it will fail sometimes and should be changed so that to_bytes() is called on data.

Note that in any case, this change is not wrong.... it's just that the code which uses it may be incorrect and would need to be fixed so that it doesn't cause an error with this new code.

@jdufresne
Copy link
Contributor Author

>>> io.BytesIO(u'test').read()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: expecting a bytes object, got unicode

If code is passing Unicode strings, then I think BytesIO (whether from six or io) is the wrong choice. If the incoming argument is always Unicode, then io.StringIO should be used. If the incoming argument is always the native str type (a byte string on Python 2, a Unicode string on Python 3), then six.StringIO should be used.

@abadger
Copy link
Contributor

abadger commented Sep 10, 2018

@jdufresne As I said:

Note that in any case, this change is not wrong.... it's just that the code which uses it may be incorrect and would need to be fixed so that it doesn't cause an error with this new code.

@abadger
Copy link
Contributor

abadger commented Sep 10, 2018

Also, in our code base, six.StringIO should not be used with native string type, it should be used with the text type only (just as io.StringIO would be)

@Qalthos
Copy link
Contributor

Qalthos commented Sep 10, 2018

I took a trip through paramiko's source just to be sure, but I feel confident that _ssh_shell.recv() should only return bytes. Thanks for the catch!

@Qalthos Qalthos merged commit ef67d40 into ansible:devel Sep 10, 2018
@ansible ansible locked and limited conversation to collaborators Jul 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.8 This issue/PR affects Ansible v2.8 needs_info This issue requires further information. Please answer any outstanding questions. needs_template This issue/PR has an incomplete description. Please fill in the proposed template correctly. networking Network category small_patch support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants