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

Address issue #216: rename the klass argument to create_protocol. #236

Merged
merged 2 commits into from
Jul 30, 2017

Conversation

cjerdonek
Copy link
Collaborator

This addresses issue #216.

One minor note: you'll notice in the signatures that I made the default value None instead of the default class. The reason is that this makes the API somewhat friendlier when it comes to composing functions, etc. For example, if an end user defines a wrapper function and wants the same default, they can get the same default by passing None. They don't need to hard-code the default value in a second place. Secondarily, when it comes to deciding between create_protocol and klass, this makes it easier to know which arguments were explicitly passed.

cjerdonek and others added 2 commits July 29, 2017 14:36
* Clarify documentation wording a bit.
* Make the backwards-compatibility logic more explicit (and removable).
* Move klass with legacy_recv, the other backwards-compatibility shim.
@aaugustin aaugustin force-pushed the issue-216-remove-klass branch from 5c7f89b to 39cec14 Compare July 29, 2017 12:56
@aaugustin
Copy link
Member

Excellent! 👍 for using None as a default value.

I suggested a few tweaks. If you're OK with them, please merge.

Thanks!

@cjerdonek cjerdonek merged commit e1c3c31 into master Jul 30, 2017
@cjerdonek cjerdonek deleted the issue-216-remove-klass branch July 30, 2017 09:03
@cjerdonek
Copy link
Collaborator Author

Thanks, Aymeric!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants