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

dahua profile setting #246

Merged
merged 18 commits into from
Jan 20, 2025
Merged

dahua profile setting #246

merged 18 commits into from
Jan 20, 2025

Conversation

NiklasNeugebauer
Copy link
Contributor

@NiklasNeugebauer NiklasNeugebauer commented Jan 14, 2025

feat: use substream parameter for dahua cameras

DEPRECATION: rename jovision_profile to substream in RtspCamera and RtspCameraProvider

feat: use substream parameter for dahua cameras
@NiklasNeugebauer NiklasNeugebauer added this to the 0.22.0 milestone Jan 14, 2025
@NiklasNeugebauer NiklasNeugebauer marked this pull request as draft January 14, 2025 16:27
@NiklasNeugebauer
Copy link
Contributor Author

NiklasNeugebauer commented Jan 14, 2025

I would like to rename the jovision_profile in RtspCamera to substream also.
This change would break both user code and and persistence.
Should we mark the argument as deprecated and support both first?

Suggestion:

  • warning when parameter is used in init
  • warning when parameter is changed (through set_parameter)
  • save into persistence under new name

@NiklasNeugebauer NiklasNeugebauer marked this pull request as ready for review January 17, 2025 15:41
@NiklasNeugebauer
Copy link
Contributor Author

Opinions on the deprecation?

@falkoschindler
Copy link
Contributor

I expected the decorator to hide type information of decorated functions. But as far as I can tell it seems to work. Mypy and pylint are happy too. 👍

Copy link
Contributor

@pascalzauberzeug pascalzauberzeug left a comment

Choose a reason for hiding this comment

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

will be removed in future versions

How do we want to keep track of deprecations? Keep them for x versions, for an amount of time or just until we feel like removing it?

@NiklasNeugebauer
Copy link
Contributor Author

NiklasNeugebauer commented Jan 20, 2025

Versions would be easiest to keep track of but is very arbitrary since we have neither a release schedule nor major versions.

I guess something like 5 versions ahead would still make sense though?

@falkoschindler
Copy link
Contributor

I'd suggest to tag deprecated code with # DEPRECATED and mention the version when a feature will be removed. This way we can find and clean up such code more easily in the future.

@NiklasNeugebauer
Copy link
Contributor Author

I added the version to the decorator and a comment where it is not used

@pascalzauberzeug
Copy link
Contributor

pascalzauberzeug commented Jan 20, 2025

@NiklasNeugebauer What do you think about a decorator not just for parameters but also for functions and classes?
I like the way it handles remove_in_version. I'm not a fan of the comment

def wrapper(*args, **kwargs):
if param_name in kwargs:
warnings.warn(
f'The parameter "{param_name}" is deprecated and will be removed in {"Rosys " + remove_in_version if remove_in_version else "a future version"}.',
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will look funny if remove_in_version is None:
"...and will be removed in Rosys a future version."

By the way, it's "RoSys" with an uppercase "S".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yes, that should have parentheses. Good catch

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I'm wrong. if has higher precedence, so it should already work correctly.

@NiklasNeugebauer
Copy link
Contributor Author

@NiklasNeugebauer What do you think about a decorator not just for parameters but also for functions and classes? I like the way it handles remove_in_version. I'm not a fan of the comment

I had the same thought but was too lazy. I'll add one for functions.

@NiklasNeugebauer
Copy link
Contributor Author

And I found another bug, the parameter code is really too complicated for this...

@NiklasNeugebauer NiklasNeugebauer merged commit 5cffc45 into main Jan 20, 2025
5 checks passed
@NiklasNeugebauer NiklasNeugebauer deleted the dahua_profile_setting branch January 20, 2025 17:17
@@ -73,7 +84,7 @@ async def connect(self) -> None:
return

self.device = RtspDevice(mac=self.id, ip=self.ip,
jovision_profile=self.parameters['jovision_profile'],
substream=self.parameters['jovision_profile'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it correct to access self.parameters['jovision_profile']?

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