Skip to content

Add support for arguments with Nil type#160

Merged
attwad merged 5 commits intoattwad:masterfrom
ideoforms:master
Nov 18, 2022
Merged

Add support for arguments with Nil type#160
attwad merged 5 commits intoattwad:masterfrom
ideoforms:master

Conversation

@ideoforms
Copy link
Contributor

@ideoforms ideoforms commented Nov 14, 2022

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

attwad commented Nov 15, 2022

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

@ideoforms
Copy link
Contributor Author

Done!

@attwad
Copy link
Owner

attwad commented Nov 15, 2022

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

@ideoforms
Copy link
Contributor Author

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

@attwad
Copy link
Owner

attwad commented Nov 15, 2022

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

@ideoforms
Copy link
Contributor Author

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

PeterJCLaw commented Nov 15, 2022

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 #161 which should fix the CI, though highlights has an issue on Python 3.6.

@PeterJCLaw
Copy link
Contributor

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

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

@attwad attwad merged commit 4b40a71 into attwad:master Nov 18, 2022
@attwad
Copy link
Owner

attwad commented Nov 18, 2022

All green now, 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.

3 participants