-
-
Notifications
You must be signed in to change notification settings - Fork 77
Conversation
Hello. Thanks for contributing to our project. I have noticed you made a lot of changes. Can you provide details about the functionalities you'd like to add? I guess what you are aiming for is adding additional commands and hooks in the main loop of the bot by adding "libraries". Did I get it right? |
local_lib_pipinstal.sh
Outdated
|
||
# 202001108 | ||
|
||
venv/bin/python -m pip install -e local-lib/test |
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.
We don't use pip to manage our project, since @azlux has his own auto-update engine. I think it would be messy if we choose to use pip to manage the lib install...
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 was unaware of this update method (I have auto update disabled since I manually inject code to obtain functionalities / one of my reasons for this request).
mumbleBot.py
Outdated
@@ -33,13 +34,15 @@ | |||
|
|||
|
|||
class MumbleBot: | |||
version = 'git' |
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 keep this line. Azlux has the routine for replacing this line with the correct version when doing a release. The 'git' stands for the development version and will be dealt with
Line 209 in 0140863
def get_version(self): |
local-lib/testlib/testlib/typical.py
Outdated
return inner_decorator | ||
|
||
|
||
class TestLib(): |
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 think this Lib does something like reading RSS feed. Can you elaborate more? And if this Lib does do some thing, then we should give it a meaningful name like RSSLib.
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.
This lib was pure for example/testing, I could bring it down as suggested being the example RSSLib
local-lib/testlib/testlib/typical.py
Outdated
return rssfeeds | ||
|
||
|
||
def loadmod(bot): |
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.
load_mod.
mumbleBot.py
Outdated
@@ -459,6 +505,23 @@ def _download(self, item): | |||
def loop(self): | |||
while not self.exit and self.mumble.is_alive(): | |||
|
|||
# It's posible for libs to start tasks, they are managed/monitored over here. | |||
for task in self.task_handle: | |||
name = self.task_handle['method']['task_name'] |
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 happen to be in the middle of considering optimizing the logic of the main loop to make it lighter. And now we have these dict lookups in the middle...
I think if you create a specific class like
from enum import Enum
class TaskState(Enum):
added = 0
started = 1
class TaskHandle:
def __init__(self, name, state, handle):
self.name = name
self.state = TaskState.added
self.handle = handle
...
However, I'm just wondering why you put task initialization in the main loop?
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 It's better to just release the loop of then already started thread tasks in loop once, this would also give better control of stopping/pause the task.
I currently placed it in the loop the get them running as late as possible, as they live quite on their own.
mumbleBot.py
Outdated
@@ -862,6 +924,8 @@ def start_web_interface(addr, port): | |||
# Create bot instance | |||
# ====================== | |||
var.bot = MumbleBot(args) | |||
# load local libraries from /local-lib | |||
libimport.import_all_loclibs(var.bot) |
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.
import_all_local_libs
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 that I don't have time to do a full review. I just briefly glanced through your changes and posted some comments.
I think we still need to do a lot of clean up and functionalities tests before I can merge it.
Actually, I'd appreciate if you could start an issue and discuss your plan with us before you get started...
@azlux and I are always on IRC channel #mumble at freenode.net and feel free to drop by and have some small discussion with us :)
Correct, So a.t.m I manually inject my own code to interact with my running services, what means auto update must be disabled, aging the instance. functionality like this would be the solution for that. Started issue: #229 |
mumbleBot.py
Outdated
@@ -206,22 +203,80 @@ def ctrl_caught(self, signal, frame): | |||
|
|||
self.exit = True | |||
|
|||
def get_version(self): |
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 explain this deletion?
mumbleBot.py
Outdated
self.log.info("bot: music stopped. playlist trashed.") | ||
|
||
def stop(self): | ||
self.interrupt() | ||
self.is_pause = True | ||
if len(var.playlist) > 0: |
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 explain this part?
Logics related to the loop and its state are messy and modifications, if not thoroughly tested, if prone to errors. Are you sure this will work?
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.
As you can see, this part was introduced in fceb50e, to fix a bug. I think you need to keep the part unchanged.
- catching up
Great! Thanks for the improvement. I'd suggest you check out the ABC of python, and write a template ABC class for all Libs inside libimport.py (or I think you can rename libs into plugins and rename libimport.py into plugins.py to avoid confusion). Another thing: I think you don't have to use force push in this PR. Using force push makes it hard for me to find out what is your improvement across each commit. You can just commit as usual and we will squash this PR before merging. What's your idea? |
mumbleBot.py
Outdated
@@ -459,6 +516,20 @@ def _download(self, item): | |||
def loop(self): | |||
while not self.exit and self.mumble.is_alive(): | |||
|
|||
# Release started tasks if any |
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.
What do you think of moving this part to the place between L516 and 517, outside the while loop?
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.
That would be better as there is no active monitoring by the bot.
mumbleBot.py
Outdated
for task in self.task_handle: | ||
self.log.debug(f'bot.loop: about to release task: {task}') | ||
try: | ||
if self.task_handle[task].state[0] == util.TaskState.started: |
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'm kind of confused about "started", "released" and "stopped". Can you explain a little bit?
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.
added: init
started: thread is running
released: task may start/resume it's job
paused: task waiting for released
stopped: exit/break task loop
Any renaming suggestions, or would you rather see threads just being started to do their stuff without further "control"?
As for the interaction with the threads maybe queue is something to consider here, and feed the q object some addressed TaskHandle ,
For now I will leave it up to further discussion.
I'm not known with that, I will check that out, thank you.
Ok, that would be:
/local-lib/rssplugin/rssplugin/typ_rssplugin.Rss
I know sorry, I'm not used to push PR, to me -f seemed the only way to get this PR updated again. |
Actually you can just keep pushing commits into your 'importlib' branch, and new commits will just show up here :). |
- removed TaskState
- Removed TaskState - Renamed function register_lib => register_plugin
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.
updating
- Forgot to remove variable for warning *line5 "import command" is shadowed by "command" in register_command()
# In order to get rid of another bot as being the argument at commands, a suggestion is to migrate old commands. # pack at mumbleBot.py.message_received / command_exc # self.cmd_handle[command_exc]['handle'](self, user, text, command_exc, parameter) # v # self.cmd_handle[command_exc]['handle'](bot=self, user=user, text=text, command_exc=command_exc, parameter=parameter) # - Requires modification typ_rssplugin.py commands for *args (to ignore it's bot object, since __init__ self.bot) *Example below @command('kwargs') # - Requires modification of all existing command.py commands for dealing with *args (to still obtain that bot object)
# In order to get rid of another bot as being the argument at commands, a suggestion is to migrate old commands. # pack at mumbleBot.py.message_received / command_exc # self.cmd_handle[command_exc]['handle'](self, user, text, command_exc, parameter) # v # self.cmd_handle[command_exc]['handle'](bot=self, user=user, text=text, command_exc=command_exc, parameter=parameter) # - Requires modification typ_rssplugin.py commands for *args (to ignore it's bot object, since __init__ self.bot) *Example below @command('kwargs') # - Requires modification of all existing command.py commands for dealing with *args (to still obtain that bot object)
Hello! Any update on this PR? |
Looks like this user hasn't been active on GitHub since January 3, 2021... I would love to contribute to this PR to get it merged on botamusique, though. @TerryGeng, could we get a checklist going to see what's left for this to get merged? I will look into this & review the changes as soon as I have some time. |
Basic loader for local libraries with optional looping tasks.
venv/bin/python -m pip install -e local-lib/testlib
As it will be installed as *.egg-link in venv/../../site-packages, updating is a matter of updating the files in local-lib folder.