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

Python 3 Compatibility #57

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
@seveas
Copy link
Contributor

seveas commented Feb 7, 2015

These three patches make beanstalkc compatible with python 2.4-3.4. Possibly
even 2.3, but I don't have that installed anywhere to test.

@svisser

This comment has been minimized.

Copy link
Contributor

svisser commented Feb 7, 2015

There is a warning in the docs about sys.exc_info() which is about assigning the traceback object to a variable:

Warning: Assigning the traceback return value to a local variable in a function that is handling an exception will cause a circular reference. This will prevent anything referenced by a local variable in the same function or by the traceback from being garbage collected. Since most functions don’t need access to the traceback, the best solution is to use something like exctype, value = sys.exc_info()[:2] to extract only the exception type and value. If you do need the traceback, make sure to delete it after use (best done with a try ... finally statement) or to call exc_info() in a function that does not itself handle an exception.

which does indeed appear to happen with _:

>>> try:
...   raise Exception()
... except:
...   _, a, _ = sys.exc_info()
...   print(_)
...
<traceback object at 0x1073eedd0>

I think compatibility with 2.4 should be dropped by using the as exc syntax.

@seveas

This comment has been minimized.

Copy link
Contributor

seveas commented Feb 7, 2015

Ah, thanks for that. I'll push -f a fixed version in a bit. And no, I won't drop 2.4 support in patches I submit, as I need it 😄

@seveas seveas force-pushed the seveas:python3-compatibility branch from b10209d to 5af6650 Feb 7, 2015

@arturhoo

This comment has been minimized.

Copy link

arturhoo commented Mar 18, 2015

👍

@CtrlC-Root

This comment has been minimized.

Copy link

CtrlC-Root commented Mar 30, 2015

:shipit:

EDIT: Emoticons, how do they work?

@earl

This comment has been minimized.

Copy link
Owner

earl commented May 10, 2015

Thanks for your work on this, @seveas. The continued support for Python 2.4 is much appreciated, I know from a few beanstalkc users who rely on Python 2.4; so also thanks for that on their behalf. I cherry-picked your exception handling fix, so that we have at least this part settled.

As for the other parts introducing Py3 compatibility, there's this design issue I mentioned in #13 with 8-bit transparency in combination with cross-Python 2/3 support. Your "PY3" version switch nicely manages to keep 8-bit transparency working properly on Py2, but doesn't provide similar functionality on Py3. How to do that nicely on Py3 is mainly a question of design. Since this issue seems to be little known, I'll write it up in a separate ticket. I'll leave your pull request pending, for the time being, as it's quite conceivable to implement whatever 8-bit transparency design we come of for Py3 on top of it.

@bcho

This comment has been minimized.

Copy link

bcho commented Aug 12, 2015

@earl When will you merge this pr?

@erikbern

This comment has been minimized.

Copy link

erikbern commented Aug 28, 2015

Can this be rebased/merged?

Happy to help in any form

Encode and decode data going to and coming from sockets
Under python3, we are responsible for decoding and encoding. The
implementation does not change the behaviour for python 2, and by
default uses python2-esque behaviour (using sys.getdefaultencoding())
under python 3, but lets the user override the encoding or even set it
to None to get bytes objects back. It also allows the user to pass bytes
objects to put, striking the best balance between backwards
compatibility for binary data in tubes (which can never be achieved
100%) and working nicely with text data.

Tested with python 2.4, 2.7 and 3.4

@seveas seveas force-pushed the seveas:python3-compatibility branch from 5af6650 to b626230 Oct 3, 2015

@seveas seveas force-pushed the seveas:python3-compatibility branch from b626230 to 34bbada Oct 3, 2015

@seveas

This comment has been minimized.

Copy link
Contributor

seveas commented Oct 3, 2015

Rebased and updated to include the suggestion I made on #60.

@erikbern

This comment has been minimized.

Copy link

erikbern commented Oct 29, 2015

What's going on here – can this be merged? Let me know if there's anything I can do. I'm tempted to fork this repo for now since this is holding us back from migrating our stack to Python 3

@bcho

This comment has been minimized.

Copy link

bcho commented Oct 30, 2015

@erikbern I think you can fork & rebase this patch in your sub stream, install it via editable mode.

@erikbern

This comment has been minimized.

Copy link

erikbern commented Oct 30, 2015

Yes of course – would just be nice not to have to rely on our own custom verision

@HarryR

This comment has been minimized.

Copy link

HarryR commented May 1, 2016

I've tested this with Python 3.5 and it works well, thanks.

Can we merge this?

Currently my requirements.txt uses:

-e git+https://github.com/seveas/beanstalkc.git@python3-compatibility#egg=beanstalkc
@ejlangev

This comment has been minimized.

Copy link

ejlangev commented May 16, 2016

@earl Any update on when this might be merged?

PY3 = sys.version_info[0] > 2
if PY3:
b = lambda x: isinstance(x, bytes) and x or bytes(x, 'us-ascii')
s = lambda x: x.decode('us-ascii')

This comment has been minimized.

@Rogdham

Rogdham May 27, 2016

Any reason not to do the following?

b = lambda x: x if isinstance(x, bytes) else x.encode()
s = lambda x: x.decode()

This would change the encoding from us-ascii to utf-8, but I am not sure why you chose us-ascii in the first place.

if PY3 and isinstance(body, str):
if not self._encoding:
raise ValueError('Job body must be a bytes instance when no encoding is specified')
body = bytes(body, self._encoding)

This comment has been minimized.

@Rogdham

Rogdham May 27, 2016

Why not do the following?

body = body.encode(self._encoding)
@Rogdham

This comment has been minimized.

Copy link

Rogdham commented May 27, 2016

For what it is worth, an other project facing a similar issue, redis-py, chose to go accept either bytes or str as input, and returns bytes (whereas #65 goes all the way and accepts only bytes as input).

By the way, it may be worth it to look at how they implement Python2/3 compatibility.

I think it is fine to do the encoding/decoding automagically, as long as the user is able to bypass it if she chooses to do so (here by setting encoding=None).

I've added some inline comments, but good job, take my 👍

@Rogdham

This comment has been minimized.

Copy link

Rogdham commented Jun 12, 2016

Hello @seveas, could you please bump the version number in your branch?

Also if you have some time to review my last comment, it would be appreciated :-)

@alup

This comment has been minimized.

Copy link

alup commented Jul 8, 2016

👍

@edwardmp

This comment has been minimized.

Copy link

edwardmp commented Oct 1, 2016

Would be nice if this can be merged in

@devinmatte

This comment has been minimized.

Copy link

devinmatte commented Jun 7, 2017

Is there any timeline on when me might be able to expect python3 support to be merged?

@HarryR

This comment has been minimized.

Copy link

HarryR commented Jun 8, 2017

I'm happy with this being merged... while I'm not actively using it, I have used this specific branch & revision on both 2.7 and 3.4 and 3.5 and it seems to work fine.

@Rogdham

This comment has been minimized.

Copy link

Rogdham commented Jun 8, 2017

Paging @seveas 👋: could you please take a few minutes to review my comments? 👼

I know this issue is starting to be old and all, but it would be appreciated 💛

@rwarren

This comment has been minimized.

Copy link

rwarren commented Oct 3, 2017

Any update here? Python 3 support is really needed.

@svisser

This comment has been minimized.

Copy link
Contributor

svisser commented Oct 3, 2017

@rwarren I solved it by switching to: https://pypi.python.org/pypi/beanstalkc3.

@rwarren

This comment has been minimized.

Copy link

rwarren commented Oct 3, 2017

@svisser Perfect... thanks for the PyPi link!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment