chore: resolve python2 deprecation issue #15
Conversation
zmx27
left a comment
zmx27
left a comment
There was a problem hiding this comment.
See comments. Also, it might be a good idea to change the function names to snake case, but you don't have to do that in this PR
| try: | ||
| os.environ.pop("QT_API") | ||
| except: | ||
| except ImportWarning: |
There was a problem hiding this comment.
I believe that os.environ acts like a dictionary where the keys are the names of the environment variables, and the values are their corresponding values. In that case, would it be more suitable to except a KeyError?
| @on_trait_change( | ||
| "srxconfig.[xpixelsize, ypixelsize, distance, wavelength, xdimension, ydimension]" | ||
| "srxconfig.[xpixelsize, ypixelsize, distance," | ||
| "wavelength, xdimension, ydimension]" |
There was a problem hiding this comment.
You might need to add a space either in front of wavelength or at the end of the previous line
| inst1 = Str( | ||
| "Please install pyFAI and FabIO to use the calibration function (refer to help)." | ||
| "Please install pyFAI and FabIO to use" | ||
| "the calibration function (refer to help)." |
There was a problem hiding this comment.
Same thing, I think you're missing a space
| inst2 = Str( | ||
| "(http://github.com/kif/pyFAI, https://forge.epn-campus.eu/projects/azimuthal/files)" | ||
| "(http://github.com/kif/pyFAI," | ||
| "https://forge.epn-campus.eu/projects/azimuthal/files)" |
| temp = re.findall("[-+]?\d*\.\d+|[-+]?\d+", line) | ||
| return map(float, temp) | ||
| pattern = r"[-+]?\d*\.\d+|[-+]?\d+" | ||
| return [float(x) for x in re.findall(pattern, line)] |
There was a problem hiding this comment.
The old version returns a map object, while your new version returns a list. Have you checked where this function is being used and whether that is acceptable?
There was a problem hiding this comment.
I have checked the context of the function. It wants to use the list property. Since in python2 map is more like a property of list while in py3 it becomes an iterator, we need to change it to list structure.
stevenhua0320
changed the title
|
@zmx27 Ready for another review |
zmx27
left a comment
zmx27
left a comment
There was a problem hiding this comment.
Minor comment but otherwise looks good to me
| ), | ||
| show_border=True, | ||
| label="Plasee specify the dimension of detector and size of pixel:", | ||
| label="Plasee specify the dimension of detector" |
There was a problem hiding this comment.
Maybe also fix the spelling here to make your life easier when you manually fix pre-commit errors later
There was a problem hiding this comment.
Updated in the new push, please see the update
|
@sbillinge Ready for review |
|
This looks good. There are some changes that might affect functionality, not just style/linting. Do we have a way to check that these changes didn't break anything? In principle, @zmx27 has a way of checking this by rebuilding and running the full package. It is a bit more overhead, and less complete, than having tests, but it might be good to tests that these changes dind't break anything. If you can figure out how to get that going please do it. If it is too difficult, we can merge this and figure out later how to make everything work when we have CI working. |
|
@sbillinge I'm afraid we need to check the functionality in CI since |
|
please check with @zmx27 as I thought we had this working already |
|
@stevenhua0320 Hi Steven, sorry for the late response. Essentially, the method that we came up with to test that the functionalities of |
OK, thanks for your response and if something unusual happens, notify me and I would try to figure out where we break the functionality. |
|
@sbillinge @stevenhua0320 I just finished doing the test by pip installing the |
3 participants
@zmx27 Ready for review
The same operation as in #8
-Fix F401 import not used error
-Fix E402 module level not on top level
-Fix E722 bare except issue
-Fix python2 deprecation in code.