Payload as byte stream - remove return_bytes

Review Request #4138 — Created Aug. 5, 2016 and updated

ity
pants
3755
pants-reviews
jsirois, kwlzn
  • Remove decoding of Payload since there is only one usage of it currently
  • Add TODO to decode in future if required, use as bytestream for the current use case
  • Sequel to https://rbcommons.com/s/twitter/r/3944/

travis ci - https://travis-ci.org/pantsbuild/pants/builds/149982367

  • 2
  • 0
  • 0
  • 2
  • 4
Description From Last Updated
payload here would have been bytes because of return_bytes=True decode(..) path in nailgun_protocol.py would only trigger if return_bytes==False IT ity
same as above IT ity
KW
  1. 
      
  2. src/python/pants/java/nailgun_client.py (Diff revision 1)
     
     
     

    should this get an explicit payload.decode('utf-8') treatment to preserve the current behavior?

  3. maybe a note about requiring decoding should just go in the doc string?

  4. should this also get a payload.decode() given it was return_bytes=True previously?

  5. 
      
IT
Review request changed
IT
  1. 
      
  2. src/python/pants/java/nailgun_client.py (Diff revision 1)
     
     
     

    decode will actually convert bytes into unicode string so no need here, since its already in bytes (see below: the explicit decode was removed in nailgun_protocol)

    1. right, but this was previously implicitly decoding for you (so "payload" in this snippit would've been unicode) vs now not decoding at all (and thus "payload" is now bytes) - I thought that was previously the problem (i.e. that self._stderr.write(bytes) results in a UnicodeDecodeError)?

  3. 
      
IT
  1. 
      
  2. src/python/pants/java/nailgun_client.py (Diff revision 1)
     
     
     

    payload here would have been bytes because of return_bytes=True
    decode(..) path in nailgun_protocol.py would only trigger if return_bytes==False

    1. ah, my bad - you're right. was reading this backwards.

  3. 
      
JS
  1. 
      
  2. This is an errant :param, but you could move it down just above the :returns: line as :param socket.socket socket: A connected nailgun client socket.

  3. s/:returns/:returns:/, but also the description omits the full return type, perhaps:

    :returns: the chunk type and the chunk's payload in bytes.
    :rtype: tuple of (int, bytes)
    
  4. 
      
Loading...