-
Notifications
You must be signed in to change notification settings - Fork 151
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
Modernize Python 2 code to prepare for Python 3 #60
Conversation
a7f611d
to
ca6137a
Compare
Fixed all the unicode() issues so there is now just one Python 3 FAIL in gryphon/tests/logic/libraries/arbitrage_test.py. Any ideas? |
f28696f
to
3eb5817
Compare
Updated the message above because All tests pass on both Python 2.7 and Python 3.6. |
I tested this branch with 3.7 only. Here are some quick notes :
Not sure what to do about it... Hope this helps. Don't hesitate to ping me on slack to talk about this, I d like to help to get python3 working asap :-) |
On Python 2.7, I tried to merge these changes with mine and run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine for me !
@@ -2,6 +2,8 @@ | |||
|
|||
from sqlalchemy import func | |||
|
|||
#from gryphon.lib.gryphonfury.revenue import (get_start_and_end_position, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about just remove this ?
Doubting my previous merges, I did a vanilla clone&runtests of that branch (on py2.7) and everything went fine. So I am all in support to merge this asap so I can fix my code and PRs to work with it ;-) |
As @bmoscon rightly pointed out, passing the tests in one thing but using the software is another. Is it possible for you to try out this software as a user would use it on Python 2 and 3 and let me know if it works as expected? |
I am using a modified version of gryphon, and a blind merge didn't work, so I ll need to port my changes one by one and investigate what I did that break things. Also my use of gryphon is very limited, so I am not sure it would even be that useful. |
Attempting to run
And some I don't remember reporting, probably because I suspect they happen sparsely and may be related to my specific setup :
Running the simple_market_making builtin strategy also works as it would on the current master. |
I have been trying to run tests with this branch, and I get some strange failure for imports, if I happen to run tests without using without using So I would actually put : from __future__ import absolute_import at the beginning of every module. Better safe than sorry. EDIT: To get what I mean, just run for example :
|
@garethdmm Should I closed this PR? |
Do we have anything better for supporting python3 ? |
What's the status on this and the other Python3 WIP issue that bmoscon started? Are there any points that need to be resolved between the two PRs? Can anyone summarize what's currently needed for this to move forward? I'm willing to contribute. |
I decided to try and make this work, so I did this:
And now I get:
Which looks like some kind of sqlalchemy error. I can fix it by adding
under the
...which takes more sqlalchemy-fu to fix than I know. I guess I'll go level it up, but just know that this isn't completely moribund. |
@garethdmm @zeapo @bmoscon @asmodehn Please try out this executable on Python 2.7 and see how it works. All tests pass on both Python 2.7 and Python 3.6 with 172 files modified. Python 3.7 tests must be run in allow_failures mode until rkern/line_profiler#153 (or similar) is merged and pushed to pypi.
Four undefined names still exist on both Python 2 and Python 3...
$ flake8 . --count --select=E9,F63,F7,F82 --show-source --statistics