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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

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, e.g. 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

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to wrap this line. It's shorter than 120 characters.

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