Do not return bytes, but str

[jhutar]

Fixes #484

[jhutar]

What does this error mean please?

FAIL: test_generic (tests.test_entities.GenericTestCase) [(<bound method Organization.download_debug_certificate of nailgun.entities.Organization(id=1)>, 'get')]
Check that a variety of helper methods are sane.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/SatelliteQE/nailgun/tests/test_entities.py", line 1772, in test_generic
    self.assertEqual(handlr.return_value, response)
AssertionError: <MagicMock name='_handle_response()' id='22771032522088'> != "<MagicMock name='_handle_response()' id='22771032522088'>"

[ldjebran]

This will not work in py3 this will return something like b'***'.
Please use decode('utf-8') that will work in py2 and py3.

[ldjebran]

@jhutar seems not that simple as it appear, and it seems by changing that return will affect unittests, we have to decide is nailgun default content return should be bytes or string under py3 (in case of string we have to change the unitests) if we cannot come to a consensus on this, we have to change the tests to verify the type of the returned values from nailgun response content.

[lpramuk]

NACK
str representation of bytes is
b'string' (including qoutes and b)

This has to be resolved by using .decode('utf-8')

FYI I had hard times figuring this out for SatelliteQE/automation-tools#671

[lpramuk]

Python 3.6.4

>>> str(b'bytes')
"b'bytes'"

Oops, while decode is OK:

>>> b'bytes'.decode('utf-8')
'bytes'

So please use .decode('utf-8') instead of str()

[ldjebran]

we have two alternatives
1- Change the default nailgun behavior to return always string content by putting decode('utf-8') in _handle_response.
2- Aways decode('utf-8') the returned content of nailgun

For me I prefer (and recommend) the first one as nailgun is higher lib over basic network api lib and it's logical to receive and return user ready data. Of course this need more effort as nailgun unit-tests needs to be fixed. but this will allow us to not fix all the tests that we know (and the ones not yet found) to be broken on py3.

[ldjebran]

@lpramuk @jhutar Let discuss about the alternatives tomorrow in the meeting.

[ldjebran]

But an other issue if we go with the first variant, if some function are already decoding that content they have to be fixed to not do so (as exception will be raised on py3 AttributeError , on py2 UnicodeEncodeError).

[jhutar]

Are we positive that Nailgun needs to support both Python 3 and 2? Is that a strong decision?

[jhutar]

Wont be in the mtg. There are vacations at Fry and Mon in the CZ.

[coveralls]

Coverage decreased (-0.04%) to 97.869% when pulling ebce060 on jhutar:robottelo_py36_TypeError_write_argument_must_be_str_not_bytes into 4878142 on SatelliteQE:master.

[jhutar]

So, I do not insist on what I have committed. If you want it in a different way, please let me know how.

Thank you!. Fixed.

[lpramuk]

Why don't you change _handler_reponse() ?
https://github.com/SatelliteQE/nailgun/blob/master/nailgun/entities.py#L125-L126

with something like:

if isinstance(response.content, bytes):
    return response.content.decode('utf-8')
else:
    return response.content

WYT?

[jhutar]

Done, please re-review.

[jhutar]

Oh my, it so easy to close...

[lpramuk]

Brilliant solution :-)

[lpramuk]

to other reviewers:

Please "Squash and merge" this PR.

[ldjebran]

ACK, think we need a third reviewer for this as this is making default content return as string on py3 @rochacbruno @renzon @abalakh please review

[ldjebran]

@lpramuk @rochacbruno @renzon @abalakh anyway we have string return in py2 perhaps this is not a concern but that was the default request content return.

[rplevka]

ACK
LGTM