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

proposed breaking change: change handle argument type in notification callbacks #759

Closed
dlech opened this issue Feb 6, 2022 · 4 comments · Fixed by #1020
Closed

proposed breaking change: change handle argument type in notification callbacks #759

dlech opened this issue Feb 6, 2022 · 4 comments · Fixed by #1020
Labels
Opinions Appreciated Please add an opinion on your desired resolution on this issue!

Comments

@dlech
Copy link
Collaborator

dlech commented Feb 6, 2022

This is a commonly reported problem: #724, #467 (it feels like it has come up more than this, but that is all I could find).

The solutions is to use functools.partial to wrap the callback function to include the required extra info.

But I'm wondering if it is worth making a breaking change to make it easier and more obvious. For example, we could replace the handle argument in the callback function with the BleakClient object itself. This would only break programs that are actually using the handle argument. I suspect that is not many because unless you are attaching the same callback to multiple characteristics, this information is not needed.

Originally posted by @dlech in #754 (comment)

@dlech dlech added the Opinions Appreciated Please add an opinion on your desired resolution on this issue! label Feb 6, 2022
@dlech
Copy link
Collaborator Author

dlech commented Feb 6, 2022

Having thought about this a bit more, instead of replacing the handle argument with the BleakClient object, it would make more sense to replace it with a BleakGATTCharacteristic object. Then anyone using the handle argument would only have to make a small change (basically replace handle with char in the declaration and char.handle where it is read). Then we could add "parent" properties to be able to get the parent service of the characteristic and the parent BleakClient of the service.

@koenvervloesem
Copy link
Contributor

I like your second suggestion, passing the characteristic instead of the handle to the callback function. This also feels more natural from a general point of view, as notifications are about the characteristic anyway.

@cooperlees
Copy link
Contributor

+1 on the second option. Would be great to automatically get the Client + Characteristic in a callback by default. At the moment my code has it's own class to do this, but am happy for the breaking change and small refactor.

@chill-git
Copy link

Why not let the user provide an arbitrary reference?
When they install the callback, they can specify what it is they want to get back.

dlech added a commit that referenced this issue Sep 22, 2022
This is a breaking change that changes the first argument of the
notification callback from the handle to the BleakGATTCharacteristic
object.

This has been a commonly reported problem by users (so much so it is
in our troubleshooting guide which can now be removed) and #759 has
received positive feedback for the breaking change.

It is likely that most users don't use the first argument anyway, so
in those cases, this won't actually be a breaking change.

Fixes: #759
dlech added a commit that referenced this issue Sep 22, 2022
This is a breaking change that changes the first argument of the
notification callback from the handle to the BleakGATTCharacteristic
object.

This has been a commonly reported problem by users (so much so it is
in our troubleshooting guide which can now be removed) and #759 has
received positive feedback for the breaking change.

It is likely that most users don't use the first argument anyway, so
in those cases, this won't actually be a breaking change.

Fixes: #759
dlech added a commit that referenced this issue Sep 22, 2022
This is a breaking change that changes the first argument of the
notification callback from the handle to the BleakGATTCharacteristic
object.

This has been a commonly reported problem by users (so much so it is
in our troubleshooting guide which can now be removed) and #759 has
received positive feedback for the breaking change.

It is likely that most users don't use the first argument anyway, so
in those cases, this won't actually be a breaking change.

Fixes: #759
@dlech dlech mentioned this issue Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Opinions Appreciated Please add an opinion on your desired resolution on this issue!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants