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

Add event decorators #89

Closed
wants to merge 4 commits into from
Closed

Conversation

nullzion
Copy link
Contributor

@nullzion nullzion commented Jan 6, 2016

Added event registration system, similar to command registration system. This references #84.
Added event decorator which is similar to command decorator. This references #83.

@johnmaguire
Copy link
Owner

Thanks for this, this looks nice. 👍

The plugin system is sort of undergoing some changes in the near future, so I'm not certain how long of a lifespan this code has. However, I have a couple comments to make, and if you update the PR I will definitely merge this. :)

Thanks again!

@@ -35,3 +35,20 @@ def inner(*args, **kwargs):
return inner

return wrap

def event(triggers):
Copy link
Owner

Choose a reason for hiding this comment

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

Would you mind writing a test for this decorator similar to the ones already defined in cardinal/test_decorators.py? We're not there yet with unit testing, but I've started with the low hanging fruit (decorators and exceptions!) and would like to try to keep the course. :)

@johnmaguire
Copy link
Owner

Overall this is super cool and I'm excited to not have to do the register_callback() dance inside plugin constructors / destructors anymore.

@johnmaguire
Copy link
Owner

Oh, one more thing -- if you'd like, add your name in the CONTRIBUTORS file in a separate commit. Put your name in alphabetically by first name please. 👍

@nullzion
Copy link
Contributor Author

nullzion commented Jan 7, 2016

Please let me know if this covers everything you had in mind.
Also thank you for such detailed feedback. It's been a while since I worked with Python and its such pleasure to get back to it.
If this task is done, can you please let me know if you have any priority tasks?

@johnmaguire
Copy link
Owner

Thanks a ton for this! I Flake8'd and made a couple minor tweaks. Tested it and it seems to work great! I also updated the join_on_event plugin to use it as an example of the decorator.

Glad you didn't find the feedback annoying. ;) Anything marked with the "blocking" label are the things I'd like to take care of first. Anything with the "internals" label should probably be dealt with sooner or later.

The current goal is implementing process-based plugins (I'm working on the framework for it here: https://github.com/JohnMaguire/pplugins). Huge architectural switch since each plugin will live outside the main Cardinal process. Will let us do some fancy things like implementing a web UI though! (#58)

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

Successfully merging this pull request may close these issues.

2 participants