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 support for Str datatype Enum signals #289

Merged
merged 8 commits into from
May 10, 2024
Merged

Fix support for Str datatype Enum signals #289

merged 8 commits into from
May 10, 2024

Conversation

DiamondJoseph
Copy link
Contributor

Allows for signals that are backed by enums but have a Signal datatype of str.

e.g. for the Aravis TriggerMode

Copy link
Member

@Tom-Willemsen Tom-Willemsen left a 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.

@DiamondJoseph
Copy link
Contributor Author

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.

SomeEnum(str, Enum), and str are valid datatypes for a signal with an enumeration as a backend. SomeEnum(int, Enum), SomeEnum(float, Enum), SomeEnum(Enum), float, int etc. are not.

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.

@Tom-Willemsen
Copy link
Member

SomeEnum(str, Enum), and str are valid datatypes for a signal with an enumeration as a backend. SomeEnum(int, Enum), SomeEnum(float, Enum), SomeEnum(Enum), float, int etc. are not.

Perfect. So where the docs currently talk about using Enum can you add this sort of clarification? Happy for it to be a link to a new/separate docs page if that's easiest. The relationship between datatype and backend probably also could do with explaining for an unfamiliar user.

Copy link
Collaborator

@coretl coretl left a 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?

src/ophyd_async/epics/_backend/_aioca.py Outdated Show resolved Hide resolved
src/ophyd_async/epics/_backend/common.py Show resolved Hide resolved
tests/epics/test_signals.py Show resolved Hide resolved
tests/epics/test_signals.py Show resolved Hide resolved
DiamondJoseph and others added 2 commits May 10, 2024 09:20
Co-authored-by: Tom C (DLS) <101418278+coretl@users.noreply.github.com>
Copy link
Collaborator

@coretl coretl left a 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

@DiamondJoseph DiamondJoseph merged commit 5fd7928 into main May 10, 2024
18 checks passed
@DiamondJoseph DiamondJoseph deleted the fix-str-enum branch May 10, 2024 12:33
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.

None yet

4 participants