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

Change callbacks to be passed status and result #783

Merged
merged 2 commits into from
Jan 16, 2024

Conversation

emontnemery
Copy link
Collaborator

@emontnemery emontnemery commented Jan 16, 2024

Breaking change

The signature of callbacks called when requests are completed has been changed:

  • The callbacks must now accept a bool status and an optional dict with response
  • The callbacks will be called both in case the request succeeds and when it doesn't

New callback signature:

CallbackType = Callable[[bool, dict | None], None]
"""Signature of optional callback functions supported by methods sending messages.

The callback function will be called with a bool indicating if the message was sent
and an optional response dict.
"""

Change

Callback functions are now called not only on success but also on error. This prevents failing requests from hanging forever. Also, many user facing functions which send messages to the remote device but do not accept a callback now accept a timeout parameter.

Background in PR #779

Needs #784

@emontnemery emontnemery reopened this Jan 16, 2024
@MartinHjelmare
Copy link
Contributor

The breaking change sentence sounds weird. It sounds like the callback is called when a request has changed signature.

@MartinHjelmare
Copy link
Contributor

MartinHjelmare commented Jan 16, 2024

I think you should document what the callback parameters mean, at least in the PR description and maybe in the readme.

@emontnemery emontnemery merged commit 4e9a345 into master Jan 16, 2024
1 check passed
@emontnemery emontnemery deleted the improve_callbacks branch January 16, 2024 10:51
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 participants