-
Notifications
You must be signed in to change notification settings - Fork 27
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
remove six usage #187
base: main
Are you sure you want to change the base?
remove six usage #187
Conversation
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.
This looks good.
I don't see any tests that have been touched. This may be that there were no tests for these functions, but we still want to test the behavior, so write tests that would have passed with the six but failed without it (for example, a string containing unicode) and make sure it passes again. Make sure the test fails as well before you have it pass. Do this for all the functions/methods you are deleting.
We need a news for each of these deletions.
Any package that is not used any more needs to be removed from the requirements and build.
Finally,, one thing we need to check is that @dragonyanglong worked hard to try and have this code read old ddp files whenever it could. Ones that were written by the py2 version of the program. It may be that we need all this code to actually do that and it is a mistake to remove it.
In that case, this task would be to add some comments where six is used that explain its usage. I am not sure about this....
@sbillinge Thanks for the comment. I'm not quite sure about making the test though because by removing six the code should still be working exactly the same for python3. For the deletions those are all for py2 compatibility and by removing them I changed all of the related calls to the up to date py3 versions. But indeed the tests are not fully covering the package (coverage ~50%) and if you check the test_*.py files most of the tests were also commented out and they only have def with no content. I'm not sure by they were not being coded but we might want to fill them up. |
tests are for behavior not for code. The behavior in this case is handling utf8 encoded things correctly. |
yes, let's make an issue to work on this. It is actually good training for people when they join the group, and we want this code to be good. Better yet, a series of issues, so I can assign an issue to a new person. |
|
I can close this when we have a utf8 test. Also when the issues are put up. |
Closes #181