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

Use args_from_dict in all camera classes #257

Merged
merged 12 commits into from
Feb 6, 2025

Conversation

NiklasNeugebauer
Copy link
Contributor

  • remove overwrite of from_dict in UsbCamera and MjpegCamera
  • use args_from_dict in RtspCamera

@codingpaula
Copy link
Contributor

I added some tests for this. What do you think?
If they run successfully, I'd say we're ready to merge.

@NiklasNeugebauer
Copy link
Contributor Author

NiklasNeugebauer commented Feb 5, 2025

I added some tests for this. What do you think?
Good idea.

Some notes:
I think it is quite confusing to test something that is part of Camera inside of a specific test. For example, test_storing_configurable_camera_as_dict is the only one that checks history length and base path are restored. This can be difficult to understand if a test fails and somewhat difficult to maintain.

We do not actually test the functionality that we just fixed. A base CalibratableCamera was always able to load its calibration but the subclasses overwrote its behavior.

I'll look into fixing those two points.

@NiklasNeugebauer
Copy link
Contributor Author

seems a little overkill in hindsight but that is what I ended up with

@NiklasNeugebauer NiklasNeugebauer added this to the 0.23.0 milestone Feb 6, 2025
@NiklasNeugebauer NiklasNeugebauer merged commit 48f86c4 into main Feb 6, 2025
6 checks passed
@NiklasNeugebauer NiklasNeugebauer deleted the use-camera-args_from_dict branch February 6, 2025 09:58
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.

3 participants