-
Notifications
You must be signed in to change notification settings - Fork 957
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
fix(swarm)!: make on_connection_handler_event impl mandatory. #3364
fix(swarm)!: make on_connection_handler_event impl mandatory. #3364
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you expand the changelog entry of #3264 to say that this is now mandatory?
Yeah makes sense, updated! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One wording suggestion. Otherwise looks good to me.
Make the implementation of `on_swarm_event` and `on_connection_handler_event` | ||
both mandatory. See [PR 3264] and [PR 3364]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make the implementation of `on_swarm_event` and `on_connection_handler_event` | |
both mandatory. See [PR 3264] and [PR 3364]. | |
Make the implementation of `on_connection_handler_event` | |
mandatory and thus consistent with `on_swarm_event`. See [PR 3264] and [PR 3364]. |
I find the wording hard to understand, i.e. I would expect this change to touch both methods, whereas in reality it doesn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for the confusion introduced Max. It does, it's just that for on_connection_handler_event
it was left out on #3264. Does the CHANGELOG entry make more sense to you, or do you still want me to adress it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right, my bad. Thanks for clarifying.
Make the implementation of `on_swarm_event` and `on_connection_handler_event` | ||
both mandatory. See [PR 3264] and [PR 3364]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right, my bad. Thanks for clarifying.
This pull request has merge conflicts. Could you please resolve them @jxs? 🙏 |
Description
Sorry I missed this on #3264
Notes
Links to any relevant issues
Open Questions
Change checklist