Skip to content

Conversation

@stevenhua0320
Copy link
Contributor

@cadenmyers13 Ready for review

  1. Fix F401 import not used error
  2. Fix E402 module level not on top level
  3. Fix E722 bare except issue
  4. Fix python2 deprecation in code.

@cadenmyers13
Copy link

See inline comments. Another good habit to get into when putting in PRs is to review them yourself. I typically do this by going through the code I edited, and if its not super obvious why you made the changes I leave a comment describing what I did. For example, when removing the unused import statements, I might leave an inline comment that says "removing unused imports". Does that make sense?

from pyface.api import ImageResource, SplashScreen
from pyface.api import ImageResource
from traits.api import (
Any,
Copy link
Contributor Author

@stevenhua0320 stevenhua0320 Sep 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete imports that unused

from dpx.srxplanargui.srxconfig import SrXconfig

ETSConfig.toolkit = "qt"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change deprecated "qt4" to "qt"

if sys.platform == "win32":
pyFAIdir = os.path.join(sys.exec_prefix, "Scripts")
elif sys.platform == "linux2":
elif sys.platform.startswith("linux"):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change deprecated "linux2" platform check to "linux"


def callPyFAICalibration(self, image=None, dspacefile=None):
if image == None:
if image is None:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

python2 cond check fix

temp = re.findall("[-+]?\d*\.\d+|[-+]?\d+", line)
return map(float, temp)
"""Extract all floats from a string and return them as a list."""
pattern = r"[-+]?\d*\.\d+|[-+]?\d+"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

python2 formatting issue fix

@cadenmyers13
Copy link

Nice, please see my comments too

@stevenhua0320
Copy link
Contributor Author

Nice, please see my comments too

Just to make sure did you make the comments in code review space? I could not see the comments.

@cadenmyers13
Copy link

Yeah i did. like ss below

Screenshot 2025-09-23 at 11 45 33 AM

@stevenhua0320
Copy link
Contributor Author

Yeah i did. like ss below

Screenshot 2025-09-23 at 11 45 33 AM

No, I could not see it on my side...

@stevenhua0320
Copy link
Contributor Author

Yeah i did. like ss below

Screenshot 2025-09-23 at 11 45 33 AM

It might because the review comment is "pending" to submit?


def cpReftext(self):
cpToClipboard(self.reftext)
return

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this supposed to be indented?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is supposed to be indented, as it has self.reftext, thenthereftext` str variable should be indented. I have made that change.

show_border=True,
label="Plasee specify the dimension of detector and size of pixel:",
label="Plasee specify the dimension of detector"
"and size of pixel:",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this syntax valid? do you need a + or to wrap it in parenths?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved in recent push

),
label="Please specify the wavelength and distance between sample and detector:",
label="Please specify the wavelength and"
"distance between sample and detector:",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as below

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved in recent push

@cadenmyers13
Copy link

oh true, thanks. you should be able to see it now

@stevenhua0320
Copy link
Contributor Author

@cadenmyers13 All the comments are edited, please see the new version and ready for review

Copy link

@cadenmyers13 cadenmyers13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more comment

show_border=True,
label="Plasee specify the dimension of detector and size of pixel:",
label="Plasee specify the dimension of detector"
+ "and size of pixel:",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure you add a space here and to other similar areas (i.e. + " and... instead of + "and...)

@stevenhua0320
Copy link
Contributor Author

@cadenmyers13 Have changed for label, ready for review.

@cadenmyers13
Copy link

@stevenhua0320 Thanks.

@sbillinge ready for review by you

@sbillinge
Copy link
Contributor

@stevenhua0320 @cadenmyers13 This PR seems to be into the getegui branch. Why is that? Shouldn't it be into migration? Was it originally branched off the getegui branch? We want to be using migration as the default branch, so sync from migration then make a new branch and then make the PR into migration.

@stevenhua0320
Copy link
Contributor Author

@sbillinge Because getegui branch was set to be the default so I mistakenly did all the commits on that. I would take these commits as a reference and once the work is done on new branch I coudl sequentially close it.

@stevenhua0320
Copy link
Contributor Author

Closed #8 as not merge to right branch

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.

3 participants