Opened 10 months ago

Last modified 6 months ago

#9343 enhancement reopened

Add a ensureBytes() helper function

Reported by: Craig Rodrigues Owned by: Craig Rodrigues
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch:
Author:

Description (last modified by Craig Rodrigues)

It's useful to have a utility function to convert from unicode to bytes.

https://github.com/twisted/twisted/pull/961

There are several places in the Twisted source where a pattern like:

` if isinstance(s, unicode):

s = s.encode("ascii")

`

This attempts to encapsulate this pattern in a function.

Based on this feedback from an earlier PR, I did not put this in twisted.python.compat: https://github.com/twisted/twisted/pull/936#discussion_r153816654

My motivation for this is to fix some problems I found in twisted.web when I found this problem when trying to port https://github.com/LeastAuthority/txkube to Python 3: https://github.com/twisted/twisted/pull/936#discussion_r153823007

Specifically if I have: ` networkString(foo) ` if foo is of type bytes on Python 2, it will work fine, but if foo is of type bytes on Python 3, this networkString will throw an exception.

If this code goes in, I would also like to delete the _ensureBytes() private function in https://github.com/twisted/twisted/blob/trunk/src/twisted/web/http.py#L1154

Change History (17)

comment:1 Changed 10 months ago by Craig Rodrigues

Keywords: review added

comment:2 Changed 10 months ago by Craig Rodrigues

Description: modified (diff)

comment:3 Changed 10 months ago by Jason Litzinger

Owner: set to Jason Litzinger

Taking for review.

comment:4 Changed 10 months ago by Jason Litzinger

Keywords: review removed
Owner: changed from Jason Litzinger to Craig Rodrigues

comment:5 Changed 10 months ago by Jean-Paul Calderone

Please write more words on tickets. Like ... why do this?

comment:6 Changed 9 months ago by Craig Rodrigues

Description: modified (diff)

comment:7 Changed 9 months ago by Craig Rodrigues

Keywords: review added

comment:8 Changed 7 months ago by Craig Rodrigues

Description: modified (diff)
Summary: Add a toBytes() helper functionAdd a ensureBytes() helper function

comment:9 Changed 7 months ago by Craig Rodrigues

Submitting a new PR for this, renaming function to ensureBytes()

https://github.com/twisted/twisted/pull/961

Last edited 7 months ago by Craig Rodrigues (previous) (diff)

comment:10 Changed 7 months ago by Craig Rodrigues

Description: modified (diff)

comment:13 Changed 7 months ago by Craig Rodrigues <rodrigc@…>

Resolution: fixed
Status: newclosed

In 30b28fbc:

Merge pull request #961 from twisted/9343-rodrigc-ensurebytes

Author: rodrigc
Reviewer: adiroiban
Fixes: ticket:9343

Create a utility function for converting unicode to bytes

comment:14 Changed 6 months ago by mark williams

Keywords: review added

comment:15 Changed 6 months ago by Amber Brown <hawkowl@…>

In e94e592:

Revert 9343-rodrigc-ensurebytes: Revert "Create a utility function for converting unicode to bytes"

Reverted as it creates a new public API which conflicts with names in the Python standard library, and the implementation allowed for some subtle and possibly hard to debug issues.

Required review due to a non-clean revert (as later code added dependencies to this function).

Author: markrwilliams
Reviewers: adiroiban, hawkowl
Reopens: ticket:9343
Closes: ticket:9401

comment:16 Changed 6 months ago by hawkowl

Resolution: fixed
Status: closedreopened

Hmm. the ticket didn't do this automatically. Reopening, as this is a revert.

We should decide if we want this functionality or whether it allows potential bugs. I'm a -0.5, because it may be useful on some limited external APIs for compat reasons, but I also believe it makes the conversion too implicit for future debugging and maintainability, even if it is shorter.

comment:17 Changed 6 months ago by hawkowl

Keywords: review removed
Note: See TracTickets for help on using tickets.