remove six usage #187
remove six usage #187
Conversation
There was a problem hiding this comment.
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