-
Notifications
You must be signed in to change notification settings - Fork 5k
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
use str for hook key #1711
use str for hook key #1711
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1711 +/- ##
===========================================
+ Coverage 39.62% 50.74% +11.12%
===========================================
Files 57 57
Lines 6006 6020 +14
Branches 1338 1464 +126
===========================================
+ Hits 2380 3055 +675
+ Misses 3433 2718 -715
- Partials 193 247 +54
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Looks good!
Before proceeding to merge I just want to understand the cause of error a bit more. See my comment: #1706 (comment) |
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.
Perhaps we can take this opportunity to rename the hookable methods:
process_last_message
-->process_last_incoming_message
process_all_messages
-->process_all_incoming_messages
For reference, the explanation is discussed in #1706 |
Unless I'm missing something, |
Let's do it in a separate PR. Could you create an issue? |
I've just checked the error in chess example notebook, and it works fine with the first commit. :) Thanks. |
* use str for hook key * bump version to 0.2.15 * remove github * disable assertion
Why are these changes needed?
Related issue number
Closes #1706
Checks