-
Notifications
You must be signed in to change notification settings - Fork 26
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: metadata handling for enums #616
Conversation
Interactively, applying this fixed the mix-up between the enum values for putting to a ("1", "2", "5") enum signal but it actually increased the number of error messages I got at startup... Before:
After:
|
Scratch that- the extra spam errors come from the signal not being fully connected when the widget comes online. Waiting for connection resolves the spam. This isn't something to overlook but it's unrelated to the problem the PR is fixing. |
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.
This looks right to me. All I have to offer is documentation type nitpicks.
Did we ever run into this on our end? Or did we just never set string
at all?
typhos/plugins/core.py
Outdated
@@ -83,6 +83,8 @@ def __init__(self, channel, address, protocol=None, parent=None): | |||
self._connection_open = True | |||
self.signal_type = None | |||
self.is_float = False | |||
self.enum_strs = () |
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 got confused for a bit about what type this actually ends up as, it's not always a tuple right? Maybe a type hint here would help, since it's buried in send_new_meta
?
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.
Let me check, I think it's always a tuple but it might also be a list
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.
It looks like enum_strs
is always casted to a tuple when we receive them from the control layer before filling the metadata
dictionary, and are again casted to a tuple in describe
. I'll annotate this as a tuple.
typhos/plugins/core.py
Outdated
except (TypeError, ValueError): | ||
value = str(value) | ||
else: | ||
raise TypeError(f"Invalid combination: {self.enum_strs=} with {self.signal_type=}") |
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.
TIL you could do this with f-strings. This ends up also printing self
, might not be the most helpful. But this is truly a nitpick
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.
Oh shoot I didn't realize this would print self... I'll make some sort of adjustment
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.
Simple rework to put the variable names in myself:
TypeError: Invalid combination: enum_strs=('zero', 'one', 'two') with signal_type=<class 'float'>
Locally at LCLS we hadn't hit this yet for a couple reasons:
|
Interestingly, as I was testing this, I found that there was also an equivalent issue for |
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.
LGTM
Description
Fix three issues related to enum/metadata handling in the
SignalConnection
part of theSignalPlugin
and implement simplified unit tests for these cases.is_string=True
#608Motivation and Context
closes #605
closes #608
How Has This Been Tested?
Unit tests were added. I originally tried to make a larger unit test where I instantiated real/fake EpicsSignal instances and connected them to the combobox widgets as reported in the issues, but I couldn't get this to work properly in a unit testing context. Rather than dump additional dev hours into this, I came up with simpler tests, possibly these could be better.
I will need to test with a real EPICS PV too.
Where Has This Been Documented?
I need to write some release notes
Pre-merge checklist
docs/pre-release-notes.sh
and created a pre-release documentation page