search

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

Default command dictionary #9

Closed wolterhv closed 3 years ago

wolterhv commented 3 years ago

I have tested this on my end and I haven't found any bugs yet. I'll just submit this pull request so you can test yourself before merging. Let me know if you find any issues! Also, naturally, let me know any other suggestions or requests you may have.

Edit: add reference to enhancement implemented by this pull request: #8

8go commented 3 years ago

I am going through it right now. Half done.

I would like to comment the default values for the parameters like markdown_convert, formatted, code, and split in commands.yaml.example. E,g, # Default value for code is false.

Maybe some lines can be removed as default is specified.

8go commented 3 years ago

Excellent @wolterhv !

And in record time :) Today is weekend, you should be resting, ha ha ha ;)

Thanks a lot. This really makes things easier to understand for other people, and also easier to use!

Fantastic! Greatly appreciated.

8go commented 3 years ago

As far as I can tell there are only 3 things left:

Anything else you can think of to clean up some more? Any other little things that should be done, are missing?

8go commented 3 years ago

Regarding your question: ( See Issue #8 )

Since commands will be provided in a dictionary, should there be a commands.yaml dictionary ready to use included? In this case, I suppose we have to have a way to know what to populate the paths node with. What is your opinion on a script to install files to sensible default locations? Would you rather have the user modify the commands.yaml and config.yaml files first, before having a usable bot?

What do you think is best? What do you suggest?

Regarding the config.yaml.example file, I think a config.yaml.example is preferable and we force the user to copy, rename and modify the file. I am inclined to think the same about the commands.yaml.example file. Make it obvious. Force the user copy/rename file and to set the paths. But I am open to suggestions. My std PATH includes all the necessary dirs, so in my personal case I would not have to set any additional paths in the yaml file. But that is just me.

But an action item results:

Interested to hear your opinion. Cheers!

8go commented 3 years ago

Another idea comes to my mind: a "reload" feature. Entering the command reload into the bot, the bot re-reads the commands.yaml file and thereby adds or drops new commands without the need to restart the bot process. Live-update of commands so-to-speak. What do you think?

wolterhv commented 3 years ago

Thanks for your replies! See my replies below.

I would like to comment the default values for the parameters like markdown_convert, formatted, code, and split in commands.yaml.example. E,g, # Default value for code is false.

Maybe some lines can be removed as default is specified.

Ok I'll look into that.

And in record time :) Today is weekend, you should be resting, ha ha ha ;)

Thanks a lot. This really makes things easier to understand for other people, and also easier to use!

Fantastic! Greatly appreciated.

I gladly use my weekends for hobby work :) And this is just a small addition to your great project!

  • see comment above: adding comments to explain to user the 4 default values, and simplify yaml.example accordingly
  • remove: help.sh. It is no longer needed, right? All that info is already inside YAML file, correct? Any reason to keep it?
  • see comment below: possibly adjust README.md file to explain to user how to "install"/"modify" the commands.yaml.example file.

Anything else you can think of to clean up some more? Any other little things that should be done, are missing?

I tried explaining what they do on the commands.yaml.example , lines 19-29 in revision adf098a. Is this what you meant? I'm not sure what formatted does so I left that one blank and forgot to update it later. Does it enable formatting of a subset of HTML?

Regarding the help.sh script, you're totally right, I forgot about that. I'll push the commit where I delete the file soon.

Regarding the commands.yaml.example file, I agree with you. It is simplest for the project to have the user just read the instructions and install and edit the YAML files as best fits them. The action here is then to update the README.md.

Regarding further cleanups and enhancements, I think I'm happy to merge this for now and maybe open some enhancement requests as the ideas come :) Off the top of my head, maybe I would propose unifying some commands which do similar things, like the tides and waves commands or those commands to retrieve cryptocurrency prices.

Another idea comes to my mind: a "reload" feature. [...]

I think this is a great idea, it can help the user debug their scripts a lot quicker than they would should they need to restart the bot every time. I like it. I think we should open a separate issue for that though.

I hope I addressed all your points. Let me know if I missed anything!

wolterhv commented 3 years ago 
I would like to comment the default values for the parameters like markdown_convert, formatted, code, and split
in commands.yaml.example. E,g, # Default value for code is false.

Maybe some lines can be removed as default is specified.

Ok I'll look into that.

I see that since there is only one call to self._os_cmd in bot_commands.py which is used to execute a command from the command dictionary, I can't think of a way to use the parameter defaults in some calls and the values from the command dictionary in others. Therefore, I'm not sure whether I can remove the DEFAULT_OPT_ constants from the definition of the CommandDict class. I also see that default values are defined in self._os_cmd and in send_text_to_room. Where would you suggest centrally located defaults to live?

8go commented 3 years ago

see comment above: adding comments to explain to user the 4 default values, and simplify yaml.example accordingly remove: help.sh. It is no longer needed, right? All that info is already inside YAML file, correct? Any reason to keep it? see comment below: possibly adjust README.md file to explain to user how to "install"/"modify" the commands.yaml.example file. missing?

I tried explaining what they do on the commands.yaml.example , lines 19-29 in revision adf098a. Is this what you meant? I'm not sure what formatted does so I left that one blank and forgot to update it later. Does it enable formatting of a subset of HTML?

I did not explain myself well. It is taken care of., It was a matter of documentation, explanation. Please see my changes in commands.yaml.example. Please review.

So, if you agree this bullet point it resolved, taken care of.

8go commented 3 years ago

Regarding the help.sh script, you're totally right, I forgot about that. I'll push the commit where I delete the file soon.

Taken care off, I just removed it. Done.

8go commented 3 years ago

Regarding the commands.yaml.example file, I agree with you. It is simplest for the project to have the user just read the instructions and install and edit the YAML files as best fits them. The action here is then to update the README.md.

Excellent. We agree. So, we fix the README.md file. I did update the README file todat. So, this is done. If you feel like it, review it.

8go commented 3 years ago

Regarding further cleanups and enhancements, I think I'm happy to merge this for now and maybe open some enhancement requests as the ideas come :) Off the top of my head, maybe I would propose unifying some commands which do similar things, like the tides and waves commands or those commands to retrieve cryptocurrency prices.

I personally see value in having these items separated and rather small, like small Lego blocks. Why? Because this was it is more customizable to the bot users. Imagine, your daughter just wants to surf and only cares about waves but your grandma only cares about walking on the beach and only cares about tides.

Or your friend A only has crypto X, your friend B only crypto Y and friend C only crypto Z.

So, I would rather NOT combine the commands so each user can get a personalized "fit".

8go commented 3 years ago

Another idea comes to my mind: a "reload" feature. [...]

I think this is a great idea, it can help the user debug their scripts a lot quicker than they would should they need to restart the bot every time. I like it. I think we should open a separate issue for that though.

I opened issue #10 So this is "done" as in "it is placed on the TODO list in issues page".

8go commented 3 years ago

I hope I addressed all your points. Let me know if I missed anything!

You did.

8go commented 3 years ago

I changed some files (command_dict.py, commands.yaml.example, etc). Please "pull" before making new changes so that we do not have to rebase or step on each others toes.

8go commented 3 years ago

A question: Why did you use lowercase "true" in Yaml file instead of uppercase "True" ? Same question for false/False.

8go commented 3 years ago 
I would like to comment the default values for the parameters like markdown_convert, formatted, code, and split
in commands.yaml.example. E,g, # Default value for code is false.

Maybe some lines can be removed as default is specified.

I see that since there is only one call to self._os_cmd in bot_commands.py which is used to execute a command from the command dictionary, I can't think of a way to use the parameter defaults in some calls and the values from the command dictionary in others. Therefore, I'm not sure whether I can remove the DEFAULT_OPT_ constants from the definition of the CommandDict class. I also see that default values are defined in self._os_cmd and in send_text_to_room. Where would you suggest centrally located defaults to live?

OK. Again I did not explain myself well. My issue is a lot simpler. All I am saying is: In file commands.yaml.example we have this

  # see also: upgrade
  check:
    regex:             "^check$|^chk$|^status$|^state$|^check .*$|^chk .*|^status .*$|^state .*$|^check.sh$|^check.sh .*",
    cmd:               check.sh
    help:              check status, health status, updates, etc.
    markdown_convert:  false
    formatted:         true
    code:              false

Here we can remove the last 2 lines. We can remove line formatted: true and line code: false because we are setting these 2 values to the default.

You agree that in our example these 2 lines can be removed? The code will do the same thing, right?

What is your opinion?

8go commented 3 years ago

Wow, that was way too lengthy. Sorry, for being so wordy. Summary:

Thanks again. Did I miss anything?

wolterhv commented 3 years ago

Wow, that was way too lengthy. Sorry, for being so wordy.

Don't worry, but thank you for the summary! Thank you also for implementing the changes to the README.md and commands.yaml.example files. I was working on some changes at the same time, so now I'm trying to merge them.

Created Issue #10 : the "reload" feature for later ...

Perfect, that shouldn't be too hard to tackle, although it may be better implemented directly into bot_commands.py.

Question: why lowercase "true"?

No particular reason, I avoid caps when I can. What style would you prefer? I see that the yaml processor is really flexible and accepts a lot of word pairs for booleans.

Thanks again. Did I miss anything?

I don't think so!

8go commented 3 years ago

Because the rest is Python, I personally would have chosen "True" to match the Python, but "true" is fine as well. We leave it at "true". Lowercase "true" is simpler :)

8go commented 3 years ago

Perfect, that shouldn't be too hard to tackle, although it may be better implemented directly into bot_commands.py. Yes, of course. The only logical place. See #10 Let's continue this thread on #10.