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

Python 2 support #1

Closed
faassen opened this issue May 4, 2016 · 5 comments
Closed

Python 2 support #1

faassen opened this issue May 4, 2016 · 5 comments

Comments

@faassen
Copy link

faassen commented May 4, 2016

Would you accept a Python 2 support PR?

It looks like not a lot of code changes are required.

I played around a little, here are my findings:

  • the Cython code just compiles and seems to work on Python 2 unchanged

  • the __all__ in the __init__.py fails, because I think __all__ in Cython is a unicode object, something Python 2 doesn't like. I'd advocate changing the __init__.py to explicitly import everything needed instead of doing a from .. import *. This way you are in full control of what's exposed in the public API.

  • the test suite relies on some features from Python 3:

    • in particular unitest.mock, which is available as mock on PyPI so that's easy
    • assertRaisesRegex which is missing. It'd need to be backported, or the test rewritten
      as the regexes used aren't complicated.
    • subTest, either backported or the test rewritten.

    I'm a fan of pytest myself so alternatively we can port the tests over over to that; it's compatible
    with Python 2 and Python 3. I'd be happier doing that instead.

  • I recommend a tool like tox so we can easily run the tests on both Pythons.

  • the Makefile is geared towards Python 3, but doesn't do all that much. I just called the build_ext command myself with Python 2. We could cruftify the makefile to support Python 2. Alternatively we could also just simply document the few standard Python commands needed; tox should care of building stuff and running stuff anyway and that's a simple command.

@1st1
Copy link
Member

1st1 commented May 4, 2016

OK, you can make a PR.

the all in the init.py fails, because I think all in Cython is a unicode object, something Python 2 doesn't like. I'd advocate changing the init.py to explicitly import everything needed instead of doing a from .. import *. This way you are in full control of what's exposed in the public API.

Good.

assertRaisesRegex which is missing. It'd need to be backported, or the test rewritten
as the regexes used aren't complicated.

Ideally, I'd like to see assertRaisesRegex ported. I'll be doing more development in httptools later (proper http protocol implementation etc), and I'm used to assertRaisesRegex.

subTest, either backported or the test rewritten.

We can write a rudimentary version for py2.

I recommend a tool like tox so we can easily run the tests on both Pythons.

OK.

the Makefile is geared towards Python 3, but doesn't do all that much. I just called the build_ext command myself with Python 2.

I'd prefer to keep the Makefile as is. Or, you can add a few more targets with a '2' suffix.

@1st1
Copy link
Member

1st1 commented May 4, 2016

BTW, there is this package: https://github.com/benoitc/http-parser/
It appears it uses the same C parser and supports Python 2.

@1st1
Copy link
Member

1st1 commented May 7, 2016

@faassen Are you working on any PR? Because if not (of if you haven't got too far) I'd like to close this issue. Since there is another similar package for Python 2, I see no point in migrating this one.

@grigi
Copy link

grigi commented May 7, 2016

assertRaisesRegex is provided by unittest2, and is only needed for Python <= 2.6
Python 2.7 already supports it: https://docs.python.org/2/library/unittest.html#unittest.TestCase.assertRaisesRegexp
It was renamed in Py3.2: https://docs.python.org/3.5/library/unittest.html#unittest.TestCase.assertRaisesRegex
Adding this to the top of the test (or py2 compatibility shim):

try:
    unittest.TestCase.assertRaisesRegex
except AttributeError:
    unittest.TestCase.assertRaisesRegex = unittest.TestCase.assertRaisesRegexp

Will make it just work.

@1st1
Copy link
Member

1st1 commented May 8, 2016

Alright, I'm closing this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants