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
Interact plugin #85
Interact plugin #85
Conversation
4bd6a69
to
1111bad
Compare
requires = ('ClientInfo', 'Inventory', 'Net') | ||
|
||
def __init__(self, ploader, settings): | ||
super().__init__(ploader, settings) |
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.
Please fix for python 2 support
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.
Oh, missed that one. Although the CI greenlit it for Python 2.7, interesting.
Also can you now add interact plugin to DefaultPlugins |
I'm also not sure where all these constants belong. |
('event', event.EventPlugin), | ||
('net', net.NetPlugin), | ||
('auth', auth.AuthPlugin), | ||
('ticker', ticker.TickerPlugin), | ||
('timers', timer.TimerPlugin), |
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.
Maybe this should be timer
instead of timers
? The rest are singular
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.
Problem is that every plugin requires it with "Timers". This would have to be changed everywhere.
But I agree, keeping it consistent would be better.
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.
Thats not how that value works, that value is just for the settings dict that gets passed into Client, so that can be changed without much consequence.
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.
I'd still like it to be the same as in Timer's pl_announce
.
Some testing still needs to be done, although I have been using it for some time now (April, wow...).