-
Notifications
You must be signed in to change notification settings - Fork 107
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
Changes in src/Utils and test/Utils_t to remove py2 compatibilities (issue #11421) #11618
Conversation
Jenkins results:
|
Jenkins results:
|
test this please |
src/python/Utils/Utilities.py
Outdated
@@ -299,3 +295,5 @@ def encodeUnicodeToBytesConditional(value, errors="ignore", condition=True): | |||
if condition: | |||
return encodeUnicodeToBytes(value, errors) | |||
return value | |||
|
|||
makeList("") |
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.
I guess this line shouldn't be here(?)
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.
yes, you are correct, I added it to debug and then forgot to delete :/
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.
@anpicci Andrea, thank you for creating this pull request. I left a comment for you along the code. In addition to that:
a) can you please remove files that are likely results from unit tests, they are: ReportEmuTestFile.txt, basicWorkload, lumiTest.json, newFile.json, nose-marker.txt, nosetests.xml
b) rename this pull request, given that it now changes all the src/python/Utils package
?
Feel free to reach out to me via mattermost if you want to discuss any of these and/or need assistance along the way (at some point we will have to squash/rebase this PR as well ;)).
Jenkins results:
|
Jenkins results:
|
test this please |
Jenkins results:
|
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.
@anpicci thanks for fixing those unit tests.
In addition to removing the basicWorkload file from this PR, I left some minor comments along the code for your consideration.
The last part will be to rebase/squash all of your commits into only 2:
- with the src/* changes
- with the test/* changes.
This is one of the procedures we recommend for rebasing/squashing: https://steveklabnik.com/writing/how-to-squash-commits-in-a-github-pull-request , but if you prefer to do this together, I am happy to. Thanks again!
Jenkins results:
|
Jenkins results:
|
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.
@anpicci there should be only a couple of extra changes required in this PR and we are ready to move forward with this (after a rebase/squashing). Please see my comments along the code. Thanks
Jenkins results:
|
Jenkins results:
|
Jenkins results:
|
Jenkins results:
|
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.
I have addressed these
first changes to MemoryCache.py (and its _t counterpart) for dmwm#11421 issue removing comments in MemoryCache.py (and its _t counterpart) References to python2 compatibility removed from all the modules in Utils and Utils_t Implementing comments from Alan Fixing bug in Utils/IteratorTools.py Fixing small flaws in Utils/IteratorTools.py Fixing small flaws in Utils/MemoryCache.py Deleting basicWorkload Fixing small flaws in Utils/MemoryCache.py Addressing Jenkins warnings
Jenkins results:
|
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.
@anpicci these changes look good to me. As we discussed today, potentially some of those dictionary iterations didn't need to be casted to a list (only when the dictionary actually changes its size that we need to do so), but it should be fine as they are as well.
I also took this opportunity to update this wiki:
https://github.com/dmwm/WMCore/wiki/python3-transition#identifying-python2-code-to-be-dropped
with the information I had collected from previous PRs and the feedback you provided as well. Feel free to update it in case I've gotten anything wrong. Thanks
Fixes #11421
Status
ready
Description
Changes in
Utils
andUtils_t
modules to remove all Py2-compatibility dependenciesIs it backward compatible (if not, which system it affects?)
YES (for python3)