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 loop parameter optional #66

Open
septatrix opened this issue Oct 29, 2020 · 3 comments
Open

Make loop parameter optional #66

septatrix opened this issue Oct 29, 2020 · 3 comments

Comments

@septatrix
Copy link
Contributor

For more information about the loop parameter see What are all these deprecated “loop” parameters in asyncio? and Passing asyncio loop by argument or using default asyncio loop.

For most use cases the loop parameter is not needed and is deprecated in most high level APIs since 3.8 and will be removed in 3.10. The main reason for its existence were a bad specification of asyncio.get_event_loop (<3.6) and poor performance (<3.7). However these are fixed now. While I am not sure about deprecating the loop parameter like it is done in the high level asyncio interfaces I think we should probably make the argument a keyword-only argument or at least optional.

This will make the API easier to understand and simpler for newcomers. However we should probably think of a good conversion plan (we are still in 0.x so semver allows breaking changes).

@rob-smallshire
Copy link
Collaborator

The loop argument to open_serial_connection is already optional, but this function supports requires keyword arguments. I think that's a reasonable solution for the other functions, as although it is a breaking change, it only requires that clients add argument names at the call site. I'm not sure we can change only loop to become a keyword-only argument, because that changes the position of any remaining positional arguments.

@septatrix
Copy link
Contributor Author

The current API is

  • create_serial_connection(loop, protocol_factory, *args, **kwargs)
  • open_serial_connection(*, loop=None, limit=None, **kwargs)
  • connection_for_serial(loop, protocol_factory, serial_instance)

whereas the API from asyncio is

  • create_connection(self, protocol_factory, host=None, port=None, *, [... / **kwargs]) (self as this is a method of the loop)
  • open_connection(host=None, port=None, *, loop=None, limit=_DEFAULT_LIMIT, **kwds)
  • connection_for_serial does not have an equivalent.

I would propose the following:

  • create_serial_connection(protocol_factory, port, *args, loop=None, **kwargs) (would like to have either port or url required now that we have connection_for_serial)
  • open_serial_connection(*, loop=None, limit=None, **kwargs) (can stay as is)
  • connection_for_serial(protocol_factory, serial_instance, *, loop=None)

@rob-smallshire
Copy link
Collaborator

Rather than changing the meaning of existing positional arguments, I propose we go for keyword-only arguments, and the make the loop argument optional.

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

No branches or pull requests

2 participants