-
Notifications
You must be signed in to change notification settings - Fork 93
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
Python3 #2966
Python3 #2966
Conversation
Very happy to see this go up as a PR! 🏆 Also the line count has taken quite a satisfying ✂️ (even accounting for the fact that |
ac57034
to
64be39a
Compare
@kinow If you get a chance could you take a look at the two unittest failures (they are both |
64be39a
to
7e67e1c
Compare
From a quick look, I guess you mean client UUID, not server UUID? (the latter is needed to uniquely identify a suite run, and the new BOM infrastructure relies on this). |
Or use asyncio? |
Looks fine to me! (from reading your commit log). |
Am looking at the issue with unit tests now, but wanted to give a big shout out to @oliver-sanders for his organization with commits. I know given the number of changes here, most comments will be probably regarding things that changed and maybe a few that broke (or maybe none! 🤞). But if anyone else is curious, have a look at how well organized the changes are. I can pretty much follow his actions from readings his commit messages and looking at the grouped changes. 👏 👏 👏 Now on the broken unit tests. I got two broken tests:
First
FWIW, the code in CPython Python that checks it seems to be this one. And
@oliver-sanders sending a pull request (oliver-sanders#6) showing what needs to be changed to that all unit tests pass. That doesn't need to be merged. You can pick up the changes and put them in a nice commit (or edit previous commit, etc) :-) this way I shouldn't mess with your current nice organisation.
Cheers |
@hjoliver Yes, I was looking at doing this yesterday, probably the best way forward. Something that occurred to me is that unless we are requesting suite
|
Yes, sorry I mean the client UUID. The ZMQ client-server I've implemented does not register clients or assign them a UUID. |
f837118
to
5fa9c77
Compare
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.
Good Work! There's loads of changes, and very few issues that I can see.
I'm only partway through the diff, and since you are still working on it I'm not going to finalize my review.
The main one that seems to recur a good deal is 2to3 messing up by adding more brackets than are needed. :(
- Read code
- bin/ diff
- other diff
-
lib/python/test_diffr.py
-
lib/python/diffr.py
- Tests
- Working locally
- Travis
- Grep for leftover python2 idioms
- cmp
- Assured myself that any automated things (Codacy) that have failed are ok - I'm waiting for you to respond to @kinow 's comments on lib/cylc/scheduler.py.
Suggest the following
https://github.com/oliver-sanders/cylc/pull/9/files
6227814
to
eea3e0b
Compare
|
||
|
||
class TestDiff(unittest.TestCase): | ||
|
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.
Probably ought to have a test to ensure that sensible error is raised if int, str, float or any other non iterable is fed to it.
fbef0d7
to
33c7e63
Compare
All minor comments. |
Codacy has a few legit unused imports, unused statements, etc. You may want to take note. |
I believe before or after this issue is merged, we can close #2647, as cylc_xdot.py is no more. |
I have added some replies to the unresolved conversations. Address as necessary. This is good to go otherwise. |
|
@hjoliver 2 approvals. Do you want to take a look of this as well? Or are you happy for this to be merged? (Note: subject to Travis CI returning a good status.) |
I'm happy to merge. I'm (sadly) still not sufficiently au-fait with Python 3 differences to contribute much, and we don't need to fine-tune at this stage (cylc-8 is still almost a year away...). ... Thanks all, brilliant 💥 |
Travis CI passed this 😁 |
Codacy issues are not important, can be addressed later. |
Nice! 💥 |
Python 2.6 -> 3.7
tests/authentication/10-remote-suite-passphrase-cache.t
- awaiting conclusion of comms: future of [communication]method #2975tests/cylc-message/00-ssh.t
- awaiting conclusion of comms: future of [communication]method #2975Related Issues
closes #2858
closes #2647
closes #2858
closes #2864
??? #2785
GUI issues which become obsolete:
closes #164
closes #1620
closes #1671
closes #1996
closes #2466
closes #2498
closes #2632
closes #2898
closes #2633
closes #2956
GUI issues which we may want to keep around for the moment:
addresses #2570
addresses #2580
addresses #2284
TODO:
Highlights:
cylc review
(awaiting tornado re-write)cylc graph --reference
cylc scan
Further Work
exit
" #2281__cmp__
logicTake a look through the commits, there have been a few changes beyond Python3.