attwad / python-osc

Open Sound Control server and client in pure python
The Unlicense
514 stars 106 forks source link

Add support for arguments with Nil type #160

Closed ideoforms closed 1 year ago

ideoforms commented 1 year ago

This PR adds support for creating and parsing messages with a Nil argument type. Arguments of Nil type are mapped to a python None (and vice versa).

I have added unit tests for the new functionality, and verified third-party compatibility by sending/receiving messages containing nil parameters with liblo.

I needed this for some features I'm developing for AbletonOSC, in which OSC endpoints need to return lists of arguments which may contain one or more nil values.

Thanks for the great library!

attwad commented 1 year ago

Thanks for the PR! Any chance you can fix the failing builds on 3.7 and above? Then I'mm merge, thanks.

ideoforms commented 1 year ago

Done!

attwad commented 1 year ago

doh, now it seems to be failing only on 3.6...

ideoforms commented 1 year ago

Ah, in my haste, I removed something that is still required for tests to pass - should be good now 🤞

attwad commented 1 year ago

sorry for the back and forth but this isn't fixed yet

ideoforms commented 1 year ago

Apologies, I am a bit baffled - it's bailing at places unrelated to my new additions, stemming from an earlier commit. It's to do with this line in osc_server.py:

    def __init__(self, server_address: Tuple[str, int], dispatcher: Dispatcher, bind_and_activate: bool = True) -> None:  # type: ignore[call-arg]  # https://github.com/python/typeshed/pull/8542

I think it could be fixed by removing warn_unused_ignores = True from setup.cfg, but that doesn't seem entirely satisfactory. Maybe the author @PeterJCLaw can shed some light on this?

PeterJCLaw commented 1 year ago

If we're seeing errors due to ignores which were previously needed now being unused (which is what warn_unused_ignores = True enables) then that suggests something has changed in mypy. This is usually a good thing as it means that mypy is now better able to check code (and the ignore is no longer needed).

In this case it could mean that the error code that needs to be ignored has changed. If that's the case and we're happy to keep ignoring whatever is wrong then a valid fix is to update the error code in use. That probably does want to happen as a separate PR though.

This showing up on an unrelated PR is unfortunately a side effect of the linting tools used in CI not being version pinned.

I've put together https://github.com/attwad/python-osc/pull/161 which should fix the CI, though highlights has an issue on Python 3.6.

PeterJCLaw commented 1 year ago

Ok, I think that the typing issues are all sorted now. If you rebase just your original commit onto master I think you should be free of typing errors.

ideoforms commented 1 year ago

Done — I don't have permissions to run the workflow myself, but hopefully this time we should be good to go. Thanks @PeterJCLaw!

attwad commented 1 year ago

All green now, thanks!