-
Notifications
You must be signed in to change notification settings - Fork 28
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 support for Str datatype Enum signals #289
Conversation
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.
The code, as far as I understand it, looks fine.
Can we add some documentation about exactly how enums are supported and which combinations of SomeEnum(str, Enum)
, SomeEnum(Enum)
etc are supported?
e.g. some other places seem to believe that str enums are always required. If that's no longer true can we say so in the docs somewhere.
The testing for this is also getting a little bit hard to follow and check that it's really checking all the cases we care about. Maybe out of scope for this issue though.
I think #137 simplifies the tests by making all of the expected descriptors much simpler to generate, I started following it up again today. |
Perfect. So where the docs currently talk about using |
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'm not convinced that we want to start returning str
from SignalRW[Enum].get_value()
, which I think this does. Can we leave the enum conversion in, but make it optional? Or make a new converter for it?
Co-authored-by: Tom C (DLS) <[email protected]>
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.
Happy to merge when the ordering issue is sorted
Allows for signals that are backed by enums but have a Signal datatype of str.
e.g. for the Aravis TriggerMode