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

Replace 2to3 with six #364

Merged
merged 3 commits into from
Nov 2, 2017
Merged

Replace 2to3 with six #364

merged 3 commits into from
Nov 2, 2017

Conversation

ob-stripe
Copy link
Contributor

r? @brandur-stripe
cc @stripe/api-libraries

This PR replaces 2to3 with six to make the library's code directly compatible with both Python 2 and 3. I'm not saying we should merge this as is but I want to at least start a discussion:

  • This adds a new dependency. However, six is one of the most popular and ubiquitous Python modules and there's a good chance our users will already have it installed.

  • Arguably, this is just trading one magic layer for another. However, I think that it is much easier to reason around six than around 2to3 because with the latter, when running under Python 3, the code that is being executed is not the code that you're seeing.

  • Additionally (and my main motivation if I'm being honest), once we stop using 2to3 we will be able to move the tests out of the main library's directory and into a dedicated directory that will not be installed when the library is packaged as a wheel.

@brandur-stripe
Copy link
Contributor

I feel pretty underqualified to comment on this, but it looks pretty good as far as I can tell.

I did some googling and found this page on the Python Wiki which suggests that six should be used only when necessary, but I think that's what you've tried to do here. This cheat sheet is pretty helpful in trying to figure out when it's necessary to invoke six and when not.

If you feel pretty confident that this is the right direction, I'd trust your judgement. The idea of 2to3 terrifies me, and I'm mostly just thankful that nothing has seriously gone wrong with using it because I'm not sure that we'd have the expertise to fix it.

Regarding the new dependency: it looks like the project has been specifically designed to be easily vendorable into your project and I notice that some big projects like Django have done just that. I'm not sure what the right direction on that one would be, but we could consider doing that as well if we're worried about a new dependency.

@ob-stripe
Copy link
Contributor Author

Very good point about vendoring six. I'd actually be in favor of that since it's just one file to add. We may even want to write our own compatibility layer by stealing just the bits that we use from six as there aren't that many.

For this PR I took a conservative approach so as not to modify existing behavior. Basically what I did was use 2to3 to identify all the places that need to be updated, then used six to make these lines version-agnostic. (I also replaced all uses of sys.version_info with six.PY2 / six.PY3 for consistency.)

If you use 2to3 on the code from this branch, you'd only get the following changes:

  • removing from __future__ import ... lines. This is not needed as these imports are just a no-op with Python 3

  • removing the u prefix from the string literals that use it. This is not needed either -- while the prefix was initially removed in Python 3, it was re-introduced in 3.3 for compatibility (it's a no-op as well). We don't support 3.0-3.2 so this is not an issue.

  • a few places where Python 2-specific symbols are used (e.g. unicode), but those lines are in an if six.PY2 block or equivalent

I'm fairly sure we could do a lot of improvements to string handling and start using Unicode ~everywhere, but we'd need to be careful about that and maybe release with a major version bump, so I left that part out for now.

@brandur-stripe
Copy link
Contributor

Very good point about vendoring six. I'd actually be in favor of that since it's just one file to add. We may even want to write our own compatibility layer by stealing just the bits that we use from six as there aren't that many.

Cool makes sense to me.

For this PR I took a conservative approach so as not to modify existing behavior. Basically what I did was use 2to3 to identify all the places that need to be updated, then used six to make these lines version-agnostic. (I also replaced all uses of sys.version_info with six.PY2 / six.PY3 for consistency.)

Clever! Sure beats running the Travis build over and over again.

But yeah, I'm pretty peaceful with the chance if you're confident it's the right direction. I'll leave the merge decision up to you.

@ob-stripe
Copy link
Contributor Author

  • Updated the first commit to vendor six with the library

    • I added a [flake8] configuration section to tox.init to exclude six.py, so that we don't have to worry about flake8 violations and can easily update six just by replacing the six.py file with the latest version
  • Added a second commit to add from __future__ import absolute_import, division, print_function at the beginning of every file

    • I excluded unicode_literals for exactly the reason listed on the wiki you linked to: "Some folks don't like to import unicode_literals because it has the potential to change your API (e.g. possibly returning unicodes where before it returned 8-bit strings in Python 2)." This is the case with (at least) our exceptions string representations. IMO we should fix this and always return Unicode but this would be a breaking change.
  • Updated the examples to make them Python 3 compatible in a third commit, and also fixed some flake8 violations and did some general cleanup (no more hardcoded API keys)

ptal @brandur-stripe

@brandur-stripe
Copy link
Contributor

I added a [flake8] configuration section to tox.init to exclude six.py, so that we don't have to worry about flake8 violations and can easily update six just by replacing the six.py file with the latest version

Good call. I definitely want to be able to just copy and paste new versions to aid with updates.

Added a second commit to add from future import absolute_import, division, print_function at the beginning of every file

Yep, that looks reasonable to me. Thanks for reading all those compatibility notes so that we can build an informed design.

Cool, this looks good to me then! Pulling it in.

@brandur-stripe brandur-stripe merged commit f948b8b into master Nov 2, 2017
@brandur-stripe brandur-stripe deleted the ob-use-six branch November 2, 2017 21:45
@brandur-stripe
Copy link
Contributor

Released as 1.73.0.

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.

None yet

2 participants