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.

Sign up for GitHub

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

Jump to bottom

on super class #2746

Merged
merged 7 commits into from
Jun 7, 2023
Merged

on super class #2746

merged 7 commits into from
Jun 7, 2023

Conversation

willmcgugan
Copy link
Collaborator

@willmcgugan willmcgugan commented Jun 6, 2023 •
edited
Loading

@on decorator now matches base classes.

Solution is much the same as #2691 with optimizations.

@willmcgugan willmcgugan mentioned this pull request Jun 6, 2023
Closed
davep
davep reviewed Jun 6, 2023
View reviewed changes
Copy link
Contributor

@davep davep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this deserves an entry in the ChangeLog, and should be marked as a breaking change too. While it's unlikely someone will have negative effects from this change, it isn't impossible.

Some mention of this should likely go into the documentation too.

darrenburns
darrenburns approved these changes Jun 6, 2023
View reviewed changes
tests/test_on.py Outdated Show resolved Hide resolved
@willmcgugan @darrenburns
Update tests/test_on.py
df1056e 
Co-authored-by: darrenburns <darrenburns@users.noreply.github.com>
@willmcgugan willmcgugan merged commit 8947dbe into main Jun 7, 2023
@willmcgugan willmcgugan deleted the on-super-class branch June 7, 2023 12:43
@davep davep linked an issue Jun 8, 2023 that may be closed by this pull request
Consider the idea of having the @on message handler capture and fire on base messages #2630
Closed
@davep davep mentioned this pull request Jun 8, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davep davep davep left review comments

@darrenburns darrenburns darrenburns approved these changes

@rodrigogiraoserrao rodrigogiraoserrao Awaiting requested review from rodrigogiraoserrao

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Consider the idea of having the @on message handler capture and fire on base messages
3 participants
@willmcgugan @davep @darrenburns