Skip to content

Python 3 compatibility#58

Merged
jdunck merged 8 commits into
jdunck:masterfrom
ryanhiebert:python3-compat
Sep 22, 2015
Merged

Python 3 compatibility#58
jdunck merged 8 commits into
jdunck:masterfrom
ryanhiebert:python3-compat

Conversation

@ryanhiebert

Copy link
Copy Markdown
Collaborator

Fixes #57

This introduces real Python 3 support. Python 3's csv module is great, but it's not reasonable to attempt to write code that is compatible with Python 2 and 3 with unicodecsv. This fixes that.

Start by running the test suite for Python 3. This involved a lot of test changes to make them Python 3 compatible, as well as writing more code into the py3 module.

Then make sure that the example in the README works for Python 2 and 3, and also add that to the test runner, so that we're sure we don't screw it up again. I had to use _ to capture a couple variables, because their output is different on Python 2 than Python 3, because the csv module in each deals with and reports write counts in the native str type.

@ryanhiebert

Copy link
Copy Markdown
Collaborator Author

@jdunck: How are you feeling about this change? Think it's reasonable to get it merged?

@jdunck

jdunck commented Sep 21, 2015

Copy link
Copy Markdown
Owner

@ryanhiebert Is the idea that under py3, the file has been opened in binary mode? I suspect there's a lot of code that currently "just works" under py2 when handing text-opened files to the csv module.

What do you think about adding a test, either at each use of UnicodeReader, or in the constructor of UnicodeReader itself, to check whether the given stream is in binary mode? One approach is fairly fiddly:

('b' in getattr(f, 'mode', '')) or isinstance(f, BytesIO)

Another approach needs to wait until the first read to be certain:

def maybe_decode(bs_maybe):
   if hasattr(bs_maybe, 'decode'):
      return bs_maybe.decode(...)
  else:
      return bs_maybe

The latter approach is likely to be slower for large files.

Anyway, this branch is an improvement, sorry it's been so long for me to give feedback.

@ryanhiebert

Copy link
Copy Markdown
Collaborator Author

Yes, in Python 3 it must be opened in binary mode, or else the encoding argument to the constructor makes no sense. I don't see any reasonable way around that for user code to be Python 3 compatible.

@ryanhiebert

Copy link
Copy Markdown
Collaborator Author

It sounds like you'd want to add that bit just to the reader? I suppose that makes some sense, though it's no longer symmetrical.

I can add that to the pull request if you like, or it could be a follow up.

@jdunck

jdunck commented Sep 21, 2015

Copy link
Copy Markdown
Owner

Eh, since this 2+3 interop is a new feature, we can just make a rule: open your files in binary mode - and document that. Fair enough?

@ryanhiebert

Copy link
Copy Markdown
Collaborator Author

That's what I had in mind.

@ryanhiebert

Copy link
Copy Markdown
Collaborator Author

I'm rebasing to master to fix the merge conflict.

@ryanhiebert

Copy link
Copy Markdown
Collaborator Author

And the tests are failing now. Working on that.

@ryanhiebert

Copy link
Copy Markdown
Collaborator Author

I went ahead and fixed the failing test caused by #39.

I also removed testing for Python 2.6, added testing for 3.5 and pypy3, and switched Travis CI to use container-based builds.

As I mentioned in #39, I'd prefer if we did not keep the dict field order by using an OrderedDict, because 1) it's an extra point of asymmetry from the csv module, and 2) it requires overriding a core method of Python 3's DictReader. However, I've gone ahead and added a commit with that change so that it'll work whichever you decide.

@jdunck

jdunck commented Sep 21, 2015

Copy link
Copy Markdown
Owner

You're right about the OrderedDict change - I've reverted that in master.

@ryanhiebert

Copy link
Copy Markdown
Collaborator Author

Feel free to drop those two commits, or I'll do it later tonight.

@ryanhiebert

Copy link
Copy Markdown
Collaborator Author

OK, I've rebased to the latest master, and removed the OrderedDict stuff. I think it's mergeable now.

Currently, we pass through the Python 3 csv module. Unfortunately,
Python 3's csv module has a fairly significantly different API, and is
not compatible with even the API given in the README. This gets the
tests passing under Python 3.3 and 3.4, and doesn't break Python 2.6,
2.7, or pypy.
@ryanhiebert

Copy link
Copy Markdown
Collaborator Author

Rebased again, cleaned up some minor stuff, and added back Python 2.6 support, since we're no longer using OrderedDict.

@jdunck
jdunck merged commit 9b0a984 into jdunck:master Sep 22, 2015
@jdunck

jdunck commented Sep 22, 2015

Copy link
Copy Markdown
Owner

Merged, thank you for your code and persistence, @ryanhiebert

@ryanhiebert

Copy link
Copy Markdown
Collaborator Author

Woohoo!

@ryanhiebert

Copy link
Copy Markdown
Collaborator Author

@jdunck : I'm obviously antsy to get this released. What other issues are you wanting to get wrapped up before a release? I'll see what I can do to help get them wrapped up.

@jdunck

jdunck commented Sep 23, 2015

Copy link
Copy Markdown
Owner

Oh, I cut a release just after merging - 0.14.1 should have your work included, and therefore have 2/3 interop. Sorry I didn't make that clear.

@ryanhiebert

Copy link
Copy Markdown
Collaborator Author

Ah, OK. I saw that you bumped the version the day before you merged, so I thought you'd also created a release then. Sorry about the confusion.

@jdunck

jdunck commented Sep 23, 2015

Copy link
Copy Markdown
Owner

... Oh, I forgot to tag the last couple releases. Lemme fix that.

@ryanhiebert
ryanhiebert deleted the python3-compat branch September 23, 2015 16:31
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

Successfully merging this pull request may close these issues.

How can I write code that work in Python 2 and 3?

2 participants