search

StevenLooman / async_upnp_client

Async UPnP Client for Python
Other
46 stars 37 forks source link

Make `async_call_action` signature accept string parameter #246

Closed ramikg closed 2 weeks ago

ramikg commented 2 weeks ago

The first line of async_call_action checks whether the action argument is a string, so it makes sense for the function signature to accept an action of type str.

I've used Union instead of | in order to support Python versions < 3.10, which this library currently stops.

StevenLooman commented 2 weeks ago

Thank you for this PR @ramikg. Great that you're pointing this out!

Though, do you need this functionality? I'd much rather remove the possibility to a string instead of the action, thus removing the lines https://github.com/StevenLooman/async_upnp_client/pull/246/files#diff-c110a3c46f87b9f3bcf2ffe5e7f7df92e8b598a5bfb74caae942bb11e32285b9R519-R520 This prevents a change in the API (albeit is it just a widening of the API). I do think this is just some very old code.

ramikg commented 2 weeks ago

I personally don't need this functionality, but this might break some existing code which ignores this type error. (E.g. I found upnp-av-control, which passes strings to this function. This specific usage probably won't break as the version is locked and they're using Renovate.)

In any case, the fix should be easy, so whatever you choose is okay.

StevenLooman commented 2 weeks ago

Thank you for pointing this out. Lets not break any existing code.

StevenLooman commented 2 weeks ago

To fix the build rename the file changes/246.bugfix.rst to changes/246.bugfix. Then I'll merge this PR.

ramikg commented 2 weeks ago

Thank you, done.

StevenLooman commented 2 weeks ago

Thanks!