Skip to content

Modernize Python 2 code to get ready for Python 3 - #19

Closed
cclauss wants to merge 2 commits into
garethdmm:masterfrom
cclauss:modernize-Python-2-codes
Closed

Modernize Python 2 code to get ready for Python 3#19
cclauss wants to merge 2 commits into
garethdmm:masterfrom
cclauss:modernize-Python-2-codes

Conversation

@cclauss

@cclauss cclauss commented Jun 22, 2019

Copy link
Copy Markdown
  • Use print() function in both Python 2 and Python 3
    • Legacy print statements are syntax errors in Python 3 but print() function works as expected in both Python 2 and Python 3.
  • Old style exceptions --> new style for Python 3
    • Old style exceptions are syntax errors in Python 3 but new style exceptions work as expected in both Python 2 and Python 3.

* Use __print()__ function in both Python 2 and Python 3
    * Legacy __print__ statements are syntax errors in Python 3 but __print()__ function works as expected in both Python 2 and Python 3.
* Old style exceptions --> new style for Python 3
    * Old style exceptions are syntax errors in Python 3 but new style exceptions work as expected in both Python 2 and Python 3.
@garethdmm

Copy link
Copy Markdown
Owner

Good stuff, I'll take a close review on this soon.

@Jamim Jamim left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @cclauss,

Why do your want to use from __future__ import print_function everywhere?
As far as I can see, it's not actually required.

Also, you need to replace iteritems() with items() for dicts.

Comment thread gryphon/data_service/pollers/orderbook/websocket/bitstamp_orderbook_websocket.py Outdated
…rbook_websocket.py

Co-Authored-By: Aliaksei Urbanski <mimworkmail@gmail.com>
@cclauss

cclauss commented Jun 23, 2019

Copy link
Copy Markdown
Author

We want to prevent compatibility regressions so we use from __future__ import print_function to change the way that Python 2 operates to make it a syntax error to add legacy print statements to these files. The directive is also required in print() calls which contain a comma or do redirection.

Try: __python2 -c "from __future__ import print_function ; print 'hi'"

On the iteritems issue, I think the comment title makes it clear that this PR is not a complete port to Python 3 but is instead some initial, safe changes.

@Jamim

Jamim commented Jun 23, 2019

Copy link
Copy Markdown

Hello @cclauss,

Thank you for the clarification!
I understand and respect your intention.
But I'm just trying to say that it looks like using from __future__ import print_function doesn't actually improve anything in terms of compatibility in this particular code base.
I just checked the whole diff once again and I didn't find any print calls that might be broken without that statement.
Am I missing something?

@cclauss

cclauss commented Jun 24, 2019

Copy link
Copy Markdown
Author

What happens when a Python 2 developer adds a legacy print statement to the codebase? It works perfectly for them and breaks things for the rest of us.

@cclauss

cclauss commented Jun 24, 2019

Copy link
Copy Markdown
Author

https://github.com/garethdmm/gryphon/pull/19/files#diff-6c80fc8f016de68c9bdd5825519d3d39R27 without the future import will print different things in Python 2 and Python 3.

@zeapo

zeapo commented Jun 24, 2019

Copy link
Copy Markdown

I agree with @Jamim, if this is the only location, it should be the only part to be refactored. Especially if the code base is meant to be fully ported to 3.x, there is no need to have all these imports.

@cclauss

cclauss commented Jun 24, 2019

Copy link
Copy Markdown
Author

I repeat. These imports change the way that python2 operates to prevent Python 2 developers from breaking Python 3 code. They make legacy print a syntax error even in python2. We are merely following the Python porting best practices section in the Python documentation called prevent compatibility regressions as commented above.

$ python2

>>> from __future__ import print_function
>>> print "Hi"
SyntaxError: invalid syntax

@bmoscon

bmoscon commented Jun 28, 2019

Copy link
Copy Markdown
Contributor

I think it might be nice to hear from @garethdmm if the port to Python3 will require backwards compatibility with Python2. If not, all those extra print imports can be done away with. I definitely hope that we can fully remove python2 support and move into modern python

@cclauss

cclauss commented Jun 28, 2019

Copy link
Copy Markdown
Author

My vote would be for a single codebase that support both Python 2 and Python 3 at least for a short period of time. This is Python porting best practice because it enables invaluable A/B testing on a common codebase. Once we are sure that we have equivalent functionality on both versions of Python, we can safely drop support for legacy Python.

@garethdmm

Copy link
Copy Markdown
Owner

My concern either way is: the codebase in its current state has a long track record that gives us a lot of confidence in its correctness, and that's really important because bugs in this system could have real-world consequences to people that are quite high. Because of that I always prefer making gradual changes over time rather than large multifaceted edits at a single moment.

I think my intuition tells me we should do everything we can to prepare for a python 3 shift within the current system, then have a short period of supporting both, and then move to just python3. I don't know what the effort-cost of that will be, and if it is much larger than doing a single major switch to python 3.

@bmoscon do you think that supporting both for a period will be a huge pain?

@bmoscon

bmoscon commented Jun 28, 2019

Copy link
Copy Markdown
Contributor

@garethdmm I dont necessarily think it will be a pain as much as I think it would be pointless given then imminent death of python2. The code will definitely be more of a mess if both are supported. What about having a python3 branch. Python2 will live in master, python3 will live in a python3 branch. At some future point in time when you consider python3 baked enough to be master, you can branch master into a legacy python2 branch and make the python3 branch master. Just my 2 cents

@cclauss

cclauss commented Jun 28, 2019

Copy link
Copy Markdown
Author

Experience says that divergent branches lead to "big bang" ports that miss the opportunity to have true A/B tests with a common codebase. This usually ends badly. The safest route (see the conservative guide to python porting) is if we have one branch where the majority of users continue to run on Python 2 and a few early adopters run the same code on Python 3 and contributors have the ability to go back and forth. The __future__ imports have been part of the Python standard library for a long time for good reason. They provide the kind of safety that @garethdmm is requesting above.

Watch the Facebook videos from the last several PyCons. Dropbox has similar presentations. These are large, critical codebases that successfully made the transition.

@bmoscon

bmoscon commented Jun 28, 2019

Copy link
Copy Markdown
Contributor

I would disagree. Python3 is not for "early adopters". This code is old and uses libraries that no longer are supported, so any changes to modernize it are going to be large. How large companies do their own python2 -> python3 transition seems 100% unrelated to this project, which is a single, small codebase, without legacy users since it was only very recently released. We can disagree all day, what matters is what @garethdmm decides is the course of action he wishes to undertake.

@cclauss cclauss closed this Jun 28, 2019
@cclauss
cclauss deleted the modernize-Python-2-codes branch June 28, 2019 22:47
@garethdmm

Copy link
Copy Markdown
Owner

@cclauss Why the close? I think this branch is good even at a minimum as a discussion tool to get the python3 migration moving.

@cclauss

cclauss commented Jun 28, 2019

Copy link
Copy Markdown
Author

@garethdmm This PR was created with tools and can be recreated once #20 has landed and Python 2.7 is passing its tests.

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.

5 participants