Skip to content
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

FIX: Line edit value setting on Python 3.10+ #1000

Merged
merged 2 commits into from
May 26, 2023

Conversation

flowln
Copy link
Contributor

@flowln flowln commented May 20, 2023

This is probably another instance of #951, though it seems like the only one caught in the test coverage, since resolving it also removes the deprecation warning for that issue (or I misunderstood something about those warnings, which is highly possible :P).

In this method, when 'channeltype' is 'int', we correctly set both the
value and 'scale' to integers. However, we didn't set the resulting
division to an integer as well. Since integer division can result in a
non-integer number, the result is sometimes (maybe all the times?) a
'float' type.

This didn't cause much trouble before Python 3.10, since Python would
automatically convert the floating-point number to an integer. At most
you got a Deprecation warning telling you that this isn't the best thing
to do. However, starting with 3.10, that conversion no longer works, so
the resulting number that gets sent to the communication channel is a
mess no longer resembling the original, causing issues.

This patch simply wraps the division into an explicit conversion to
'channeltype', avoiding the issue.

Signed-off-by: flowln <thiago.ferreira@lnls.br>
Previously this test would succeed with a deprecation warning in Python
3.9 and prior, and fail in 3.10+.

After the last commit, this should succeed with no warnings on all
Python versions.

Signed-off-by: flowln <thiago.ferreira@lnls.br>
@jbellister-slac jbellister-slac self-requested a review May 22, 2023 22:28
Copy link
Collaborator

@jbellister-slac jbellister-slac left a comment

Choose a reason for hiding this comment

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

Good catch, this does look like an instance of that issue. Thank you for the fix and additional test case!

@jbellister-slac jbellister-slac merged commit 2865d59 into slaclab:master May 26, 2023
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.

2 participants