Add Python 3 compatibility #32

dan-blanchard · Jul 11, 2014

This PR makes streamparse Python 3 compatible (issue #9) by:

Using six for iteritems and reraise
Adding from __future__ import absolute_import, print_function, unicode_literals to the top of nearly every .py file.
Replacing the few remaining print statements with print functions.
Explicitly decoding/encoding all strings sent to/from spouts and bolts using UTF-8.
Adding 3.3 to .travis.yml.
Relying on a Python 3.3 compatible fork of Fabric referred to in fabric/fabric#1050 until the official Python 3 compatible version of Fabric comes out in the next few weeks.
Removing unittest2 from test_requires when on Python 3.

I also fixed a few pylint issues and other general correctness things like:

Eliminating [] and {} as default arguments as to avoid this nonsense.
Stop importing streamparse for __version__ in setup.py to fix potential issues where the user doesn't have all prereqs already installed.
Switch to using README.rst instead of README.md and made setup.py automatically use that as the long_description for PyPI.

I have not extensively tested this beyond making sure all the unit tests pass on 2.7 and 3.3 on Travis, so let me know if I missed something.

This looks really nice and complete, @dan-blanchard. Thank you for the contribution. We'll make sure to review in detail tomorrow and assuming all is good, we'll merge promptly and cut a release!

amontalenti · Jul 11, 2014

This looks really nice and complete, @dan-blanchard. Thank you for the contribution. We'll make sure to review in detail tomorrow and assuming all is good, we'll merge promptly and cut a release!

dfdeshom · Jul 11, 2014

dfdeshom reviewed Jul 11, 2014

View changes

I've switched to using a regular expression for extracting the version number like you suggested.

dan-blanchard · Jul 11, 2014

I've switched to using a regular expression for extracting the version number like you suggested.

amontalenti · Jul 11, 2014

amontalenti reviewed Jul 11, 2014

View changes

amontalenti · Jul 11, 2014

amontalenti reviewed Jul 11, 2014

View changes

amontalenti · Jul 11, 2014

streamparse/ipc.py

+        _stdout.buffer.flush()
+        _stdout.buffer.write(wrapped_msg)
+    else:
+        _stdout.write(wrapped_msg)


This is an interesting branch. Why is it that in Py3 we need to flush first?

I'm just being extra cautious. On Python 3 you can't write bytes directly to sys.stdout, but you can write to the underlying buffer, which is what I'm doing.

I cannot for the life of me find where I saw it, but somewhere I had read that when writing to the buffer directly, you should always flush first in case there are str things buffered (from using sys.stdout.write directly). In our case, I'm not sure that would ever happen, but I was just trying to be safe.

We can certainly take it out if you think it could cause performance issues or something.

Ah hah, interesting. I didn't know about this buffer vs direct writing difference between 2 and 3.

The official docs say:

if you are writing a library (and do not control in which context its code will be executed), be aware that the standard streams may be replaced with file-like objects like io.StringIO which do not support the buffer attribute.

But I guess in this situation, we do "control the environment", so checking for the existence of buffer isn't necessary. I think your code is good as-is, let's go with that.

I reviewed and everything looks good to me. @msukmanowsky if you approve, I'll merge this.

amontalenti · Jul 11, 2014

I reviewed and everything looks good to me. @msukmanowsky if you approve, I'll merge this.

Just noticed one remaining issue: SocketServer instead of socketserver in contextmanagers.py. I'll fix that.

dan-blanchard · Jul 11, 2014

Just noticed one remaining issue: SocketServer instead of socketserver in contextmanagers.py. I'll fix that.

Okay, that should be fixed now.

dan-blanchard · Jul 11, 2014

Okay, that should be fixed now.

msukmanowsky · Jul 11, 2014

msukmanowsky reviewed Jul 11, 2014

View changes

msukmanowsky · Jul 11, 2014

msukmanowsky reviewed Jul 11, 2014

View changes

msukmanowsky · Jul 11, 2014

msukmanowsky reviewed Jul 11, 2014

View changes

msukmanowsky · Jul 11, 2014

streamparse/bolt.py

@@ -226,7 +245,7 @@ def process_batch(self, key, tups):
        """
        raise NotImplementedError()

-    def _batcher(self):
+    def _batch_entry(self):


Continue the renaming to docstring mentions of batcher please.

_batcher is still the name of thread, it's just that now there's no longer a naming clash where there is an attribute called _batcher that is a thread and a method called _batcher. Now the _batcher thread runs the _batch_entry method. I'm not sure the doc strings need to be updated. It depends on whether you think they should be talking about the method or the thread.

msukmanowsky · Jul 11, 2014

msukmanowsky reviewed Jul 11, 2014

View changes

msukmanowsky · Jul 11, 2014

msukmanowsky reviewed Jul 11, 2014

View changes

msukmanowsky · Jul 11, 2014

test/ipc/test_batching_bolt.py

@@ -69,17 +69,17 @@ def test_3_multi_ack(self):
            "task": 0,
            "tuple": ["snow white and the seven dwarfs", "field2", 3, 4.252],
        }
-        for i in xrange(5):
+        for i in range(5):


Is there a six import we can do for the xrange -> range conversion? Minor thing.

Yes, six does have such a thing. I figured it didn't really matter since it was only 5 items, but I'll switch to using six.

Had a look through @dan-blanchard and overall, things look great! Just a few small knitpicks really. Thanks for moving streamparse into the future! :)

msukmanowsky · Jul 11, 2014

Had a look through @dan-blanchard and overall, things look great! Just a few small knitpicks really. Thanks for moving streamparse into the future! :)

I made all the requested changes (except for the batcher thing, which I've commented on above).

dan-blanchard · Jul 12, 2014

I made all the requested changes (except for the batcher thing, which I've commented on above).

Awesome! Thanks again @dan-blanchard, great work and contribution.

msukmanowsky · Jul 12, 2014

Awesome! Thanks again @dan-blanchard, great work and contribution.

@dan-blanchard @msukmanowsky sorry for the delay on this, but I did cut a new streamparse release, 0.0.13, that includes Storm 0.9.2 support & Python 3 support. https://github.com/Parsely/streamparse/releases/tag/v0.0.13 (it's up on PyPI and the Clojar dependency has been updated, too)

amontalenti · Jul 17, 2014

@dan-blanchard @msukmanowsky sorry for the delay on this, but I did cut a new streamparse release, 0.0.13, that includes Storm 0.9.2 support & Python 3 support. https://github.com/Parsely/streamparse/releases/tag/v0.0.13 (it's up on PyPI and the Clojar dependency has been updated, too)

Thanks @amontalenti!

dan-blanchard · Jul 17, 2014

Thanks @amontalenti!

apogre · Mar 31, 2015

Trouble of python 2.7 users!!

Is this actually not working for you on 2.7? We're using six to make sure this works with both.

yeah, I had to change it to older version to get it running on 2.7

Could you please file an issue with a stacktrace?

out of workstation. Will do it tomorrow!

Switch to using regex for extracting version info.

6508205

Add myself to contributors list. Feeling guilty since I'm first alpha…
…betically.

948f614

dan-blanchard added some commits Jul 11, 2014

Fix SocketServer instead of socketserver in Python 3.

e288785

String formatting typo

aac8a8e

dan-blanchard added some commits Jul 12, 2014

setup.py style tweaks

e2c79c1

Remove TODO comment from bolt.py

f22a58e

Use six.moves.range in test_batching_bolt.py

d29543f

msukmanowsky added a commit that referenced this pull request Jul 12, 2014

Merge pull request #32 from dan-blanchard/master
Add Python 3 compatibility

011c670

msukmanowsky merged commit 011c670 into Parsely:master Jul 12, 2014
1 check passed

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

apogre referenced this pull request Apr 1, 2015
Closed
six.moves.socketserver not working in python 2.7 #113

Parsely/streamparse

Join GitHub today

Add Python 3 compatibility #32

Conversation

dan-blanchard commented Jul 11, 2014

dan-blanchard added some commits Jul 10, 2014

This comment has been minimized.

amontalenti commented Jul 11, 2014

dfdeshom reviewed Jul 11, 2014 View changes

This comment has been minimized.

dan-blanchard commented Jul 11, 2014

amontalenti reviewed Jul 11, 2014 View changes

amontalenti reviewed Jul 11, 2014 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

amontalenti commented Jul 11, 2014

This comment has been minimized.

dan-blanchard commented Jul 11, 2014

dan-blanchard added some commits Jul 11, 2014

This comment has been minimized.

dan-blanchard commented Jul 11, 2014

msukmanowsky reviewed Jul 11, 2014 View changes

msukmanowsky reviewed Jul 11, 2014 View changes

msukmanowsky reviewed Jul 11, 2014 View changes

This comment has been minimized.

This comment has been minimized.

msukmanowsky reviewed Jul 11, 2014 View changes

msukmanowsky reviewed Jul 11, 2014 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

msukmanowsky commented Jul 11, 2014

dan-blanchard added some commits Jul 12, 2014

This comment has been minimized.

dan-blanchard commented Jul 12, 2014

This comment has been minimized.

msukmanowsky commented Jul 12, 2014

msukmanowsky added a commit that referenced this pull request Jul 12, 2014

Hide details View details msukmanowsky merged commit 011c670 into Parsely:master Jul 12, 2014 1 check passed

1 check passed

This comment has been minimized.

amontalenti commented Jul 17, 2014

This comment has been minimized.

dan-blanchard commented Jul 17, 2014

This comment has been minimized.

apogre commented on e288785 Mar 31, 2015

This comment has been minimized.

dan-blanchard replied Mar 31, 2015

This comment has been minimized.

apogre replied Mar 31, 2015

This comment has been minimized.

dan-blanchard replied Mar 31, 2015

This comment has been minimized.

apogre replied Mar 31, 2015

apogre referenced this pull request Apr 1, 2015

dfdeshom reviewed Jul 11, 2014

View changes

amontalenti reviewed Jul 11, 2014

View changes

amontalenti reviewed Jul 11, 2014

View changes

msukmanowsky reviewed Jul 11, 2014

View changes

msukmanowsky reviewed Jul 11, 2014

View changes

msukmanowsky reviewed Jul 11, 2014

View changes

msukmanowsky reviewed Jul 11, 2014

View changes

msukmanowsky reviewed Jul 11, 2014

View changes

msukmanowsky merged commit `011c670` into Parsely:master Jul 12, 2014
1 check passed

apogre commented on `e288785` Mar 31, 2015