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

Make async_call_action signature accept string parameter #246

Merged

Conversation

ramikg
Copy link
Contributor

@ramikg ramikg commented Nov 4, 2024

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.

@ramikg ramikg force-pushed the hotfix/async-call-action-type branch from b6e3179 to 723a115 Compare November 4, 2024 20:22
@ramikg ramikg force-pushed the hotfix/async-call-action-type branch from 723a115 to a647799 Compare November 4, 2024 20:24
@StevenLooman
Copy link
Owner

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
Copy link
Contributor Author

ramikg commented Nov 5, 2024

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
Copy link
Owner

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

@StevenLooman
Copy link
Owner

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

@ramikg
Copy link
Contributor Author

ramikg commented Nov 7, 2024

Thank you, done.

@StevenLooman StevenLooman merged commit c63b2d3 into StevenLooman:development Nov 7, 2024
11 checks passed
@StevenLooman
Copy link
Owner

Thanks!

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