-
Notifications
You must be signed in to change notification settings - Fork 264
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
Sweep of Python 2 compatibility features #801
Comments
Hi! I'm pretty new to open source projects even though I've been programming for a while now. I'm interested in contributing to this project and I thought this issue might be a good place to start. Do you have any suggestions for how I should tackle this? |
Hi @rkty13! Welcome! I think this issue would be a good place to start indeed :) To give a bit of background (and apologies if this is all known), this library used to support python 2 but now only supports python 3. In the past, to ensure compatibility we had to do certain little things that aren't needed anymore. For example, in python 2, division was always integer division so So this issue is to refactor/clean up code to get rid of all the extra things we were doing to ensure the library worked fine with python 2. The idea is to simply go through the source code and remove them. For example in
That can be replaced with:
So you could simply remove that and all the tests should still pass (there's information on running the tests here: http://axelrod.readthedocs.io/en/latest/tutorials/contributing/strategy/running_tests.html). That's just one example where I'm talking about float division but there will be other little things here and there (including in the strategy files: I hope that's helpful, but don't hesitate to ask if not. You can usually get one of us on the gitter channel if you want to have a quick chat: https://gitter.im/Axelrod-Python/Axelrod We take pride in being helpful to newcomers (to programmer and/or open source) so please let us know how we can help :) The full documentation might also be helpful: http://axelrod.readthedocs.io/en/latest/tutorials/contributing/index.html |
Thank you for the pointer! I guess what I really meant to ask though was if I should do this all in a new branch in this repo or if I should fork the project first and then submit a pull request. Sorry about the confusion! |
In a fork is best, since you probably can't push to this repository. |
Thank you, I'll get started on that now! |
For calling the methods of a superclass, which would be the best way to do so for Python 3? In https://github.com/Axelrod-Python/Axelrod/blob/master/axelrod/match_generator.py#L224 this is written:
However, in the same file (https://github.com/Axelrod-Python/Axelrod/blob/master/axelrod/match_generator.py#L263), this is written:
which makes use of old-style classes from Python 2. From the Python 3 docs,
Should I only change the method calls using old-style classes, or should I change everything to have a consistent form? |
My vote would be to change them all to the preferred py3 format. |
By that do you mean the zero arguments format? |
Yep. |
In https://github.com/Axelrod-Python/Axelrod/blob/master/axelrod/random_.py, there are two functions called However, when I try replacing, for example, the |
The typical reason for these tests is to make sure that all the possible behaviors for stochastic strategies are seen, so ideally we'd find a new random seed in each case that makes the test pass. There may be a few tests that this is difficult to do for so we may want to substitute a different test. If there are some tricky cases just skip them for now and we'll decide how best to fix them on a case by case basis. A separate PR for this specific py2->3 change is probably a good idea. |
Most of the changes have been made in #818. Some things I could not change:
Overall I think I have caught most of the differences between Python 2 and Python 3. |
Thanks @rkty13!
I suggest we close this issue and potentially open issues for those two remaining points? |
It's really just one issue though right? There's not much we can do about the second one. |
Yup! And actually I'm not sure we do want to get rid of |
+1 let's close and leave |
Have opened #826 which tweaks the docstring for |
There are various things in the library that can now be simplified as we no longer support py 2.
For example various computations had to be explicitly done as float division. These can be removed now :)
The text was updated successfully, but these errors were encountered: