Skip to content

Conversation

@Tieqiong
Copy link
Contributor

@Tieqiong Tieqiong commented Aug 21, 2024

Closes #181

@sbillinge
Copy link
Contributor

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

Copy link
Contributor

@sbillinge sbillinge left a 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....

@Tieqiong
Copy link
Contributor Author

Tieqiong commented Aug 22, 2024

@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
Copy link
Contributor

@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
Copy link
Contributor

sbillinge commented Aug 23, 2024

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
Copy link
Contributor

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
Copy link
Contributor

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

@Tieqiong Tieqiong mentioned this pull request Aug 30, 2024
58 tasks
@sbillinge sbillinge merged commit 3975fd5 into diffpy:main Sep 30, 2024
@Tieqiong Tieqiong deleted the six branch December 21, 2024 02:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove "six" library for Python 2 compatibility

2 participants