8go / matrix-eno-bot

An admin and personal assistence Matrix bot built on matrix-nio and nio-template
Apache License 2.0
88 stars 15 forks source link

New feature: Command dictionary #1

Closed wolterhv closed 3 years ago

wolterhv commented 4 years ago

The introduced feature allows the user to define in a YAML file commands, their help strings and the paths which should be added to the runtime PATH environment variable in order to execute them. This allows users to extend their bot without having to modify the source files. This is positive because then the users can freely pull the latest updates from the git repository without needing to merge changes, which can be painful. Additionally, new commands and their help strings are very easy to add.

When the user runs the help all or help commands command, the commands and their help are included in the reply string, in the same format currently used by the help.sh script.

8go commented 3 years ago

First of all, sorry for picking this up with so much delay. I apologize.

What a great idea @wolterhv. Excellent job. This will be helpful for many people.

Thank you @wolterhv for all your effort. It is truly appreciated! Thank you so much.

Excellent PR. Thanks again!

wolterhv commented 3 years ago

Don't worry :) You're very welcome, thanks for merging it! I'm very glad you find it helpful. Let me know if you'd like some change so that it harmonizes better with your code.

8go commented 3 years ago

@wolterhv : Any contribution, any additional feature is welcome. This week I have time so I will respond immediately.

Any advances you made please share them and I will look them over and merge the PRs. Yes, I think it would be great for the bigger community to sync up your efforts so that both of us and the rest of the world can run the same code base.

So, in short please share everything you think is useful and I get right on it.

wolterhv commented 3 years ago

What do you think about moving the existing commands to a command dictionary? I'm thinking it could simplify the bot_commands.py script. Those commands which don't use _os_cmd could be kept, but a lot of conditional cases could be removed. The command dictionary could be extended to support regex expressions, lest that functionality gets lost in the process.

8go commented 3 years ago

Hi @wolterhv

I like your idea. Simplifying is always good. And yes I agree with you there is definitely room for simplification here and some code cleanup. There should only be one way of doing things, and not 2. And the better way is the way that you contributed and implemented, the dictionary-way.

At the same time I think it would would be important to not lose any functionality. So, I think it would make sense to keep the regex functionality (i.e. put regex capability into the dictionary stuff). Also the dictionary should then be extended to not only hold cmd and help but to add items like

What do you @wolterhv suggest to do with the ls and date commands in bot_commands.py? Obviously I don't use them, but they are there as a template for people to add platform-independent code for Win, Mac, Linux. Lines 86-164. Remove completely?

echo and whoami (lines 541-561) can be removed from bot_commands.py and placed as tiny .py or .sh scripts into the script directory. What do you think about that? What do you suggest? Better ideas?

Keep only the show_help() function and remove pretty much everything else? Is that the vision? Your opinion?

In summary: great idea! If you have time, please go for it! I will appreciate it and many others as well. I see many benefits:

thanks @wolterhv for the idea and suggestion, I like it.

wolterhv commented 3 years ago

Hi 8go,

I'm very pleased that you like the idea. I'll work on it when I get a chance. I'll probably propose a yaml syntax to get your feedback on it.

About the ls and date commands, I think we can provide basic, cross-platform python versions of those commands. I think ls cold be implemented with os.listdir and date with datetime.now. What do you think? These could be standalone scripts provided with the package and referenced in the command dictionary. The user may override them with their OS equivalent if they wish. If you want the user to have access to the conditional logic to use OS commands depending on host OS type, we could forgo the python built-ins I suggested and use your already existing code. Your choice really.

I am in favour of moving the echo and whoami functions to separate scripts too.

These changes look simple enough, maybe I'll make a pull request soon :)

8go commented 3 years ago

Hi @wolterhv

Love it. I agree on everything.

Re: ls and date: it sounds a bit to ridiculous to have tiny Python scripts for them. I would just remove them. And the user (e.g. in the template) can chose an OS-dependent script/command for it like ls and date on Linux. So, in short, I suggest to remove them from the bot_commands.py file (and NO scripts). If you agree, then just purge the ls and date code..

Re: echo and whoami: Yes, tiny separate scripts please in the scripts directory. Because I think users might want to customize those to their needs/likes.

bot_commands.py will become as minimal as possible.

dictionary-related code will be as smart as possible. And as much as possible in YAML so it can be changed on-the-fly also by people who have no idea of Python. As much control as possible through YAML configuration (instead of Python code).

I think we see the things from the same angle.

Thanks for volunteering! Greatly appreciated!