Skip to content

Conversation

dbirman
Copy link
Member

@dbirman dbirman commented Jul 8, 2025

PR moves OlfactometerConfig from stimulus.py to configs.py, leaving just the stimulus_name field in the stimulus classes.

@dbirman dbirman linked an issue Jul 8, 2025 that may be closed by this pull request
@dbirman dbirman requested a review from saskiad July 8, 2025 15:58
Copy link
Collaborator

@saskiad saskiad left a comment

Choose a reason for hiding this comment

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

Don't you need to add OlfactometerConfig to the Acquisition configurations list?

@dbirman
Copy link
Member Author

dbirman commented Jul 9, 2025

Don't you need to add OlfactometerConfig to the Acquisition configurations list?

Yes good catch

@dbirman
Copy link
Member Author

dbirman commented Jul 9, 2025

  • Add a test that checks that all subclasses of DeviceConfig are options in the configurations Union

@dbirman dbirman requested a review from saskiad July 10, 2025 01:49
Copy link
Collaborator

@saskiad saskiad left a comment

Choose a reason for hiding this comment

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

I think it looks good - two things that might be worth thinking about but aren't deal breakers.

@saskiad
Copy link
Collaborator

saskiad commented Jul 18, 2025

@dbirman did this get merged? Do we want it merged?

@dbirman
Copy link
Member Author

dbirman commented Aug 6, 2025

  • Deprecate old class to avoid making a breaking change
  • Add new class to Union[]

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.0] OlfactometerConfig

2 participants