Skip to content

Improve sync_wrapper's definition so that we get better typing#2178

Closed
Ten0 wants to merge 6 commits intoAppDaemon:devfrom
Ten0:improve_sync_wrapper_typing
Closed

Improve sync_wrapper's definition so that we get better typing#2178
Ten0 wants to merge 6 commits intoAppDaemon:devfrom
Ten0:improve_sync_wrapper_typing

Conversation

@Ten0
Copy link
Copy Markdown
Contributor

@Ten0 Ten0 commented Dec 14, 2024

Before, calling functions such as self.get_state or self.turn_on with anything else but the required type (in these two cases, str) would not lead to a type error, but rather to a runtime error long down the chain.

This now properly fails to typecheck.

Argument of type "whatever" cannot be assigned to parameter "entity_id" of type "str"

Ten0 added 6 commits December 14, 2024 03:03
Because otherwise the type will enforce that the caller checks which variant it is, which is too verbose.

Ultimately this probably means that we shouldn't overload methods like this, but instead should have a separate interface for the non-async case where all methods are explicitly non-async, so that we can remove those annotations from the async ones and then the typer can help us to not forget "await".
This would prevent from calling them with e.g. `str | None` with the previous changes.
@jsl12 jsl12 self-assigned this Apr 17, 2025
@Ten0
Copy link
Copy Markdown
Contributor Author

Ten0 commented Apr 23, 2025

Ah yeah now it's called sync_decorator, and the issue is the other way around... Before we were losing all the typing, which was error-prone on arguments, whereas now the typer keeps thinking that the function returns a coroutine, although it doesn't if running in a non-asynchronous context:

image
image
image

@jsl12 since you self-assigned... this looks like something that probably wants to get fixed before the release.

@jsl12 jsl12 closed this May 21, 2025
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