search

diffpy / diffpy.pdfgui

graphical user interface for real space structure refinement to PDF
Other
20 stars 29 forks source link

remove six usage #187

Closed Tieqiong closed 4 weeks ago

Tieqiong commented 2 months ago

Closes #181

sbillinge commented 2 months ago

@Tieqiong I changed "See #181" in the opening comment to "Closes #181". This will automatically close that issue when this PR merges.

Tieqiong commented 2 months ago

@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.

sbillinge commented 2 months ago

@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.

tests are for behavior not for code. The behavior in this case is handling utf8 encoded things correctly.

sbillinge commented 2 months ago

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.

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.

sbillinge commented 2 months ago

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.

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. For example, one issue per file that needs tests.

sbillinge commented 2 months ago

I can close this when we have a utf8 test. Also when the issues are put up.