🏗️ Structure refactor T0 (discussion 31)

[VForiel]

Discussion 31

ToDo List:

• Plugin main file
• Doc grouping
• Config grouping
• Option (enabled by default) to install dependencies in a venv
• Move plugin database model
• Setup.py
• I18N public singleton (no more in a cog)
• Sconfig public singleton (no more in a cog)
• Logs overhaul
• Start.py simplification

[ascpial]

You should add the LRFutils module to the requirements.txt file (so I can at least test the code)

[ascpial]

I don't think storing the configuration in a python file is a good idea.

From a code perspective, a python file is not ideal: importing the file to read the configuration will miss default value if the user decide to empty the file; there is also security concerns: if a user give his configuration file to another user, he can insert code that will be executed in the workspace of the second user, generally runing code in the configuration is a bad idea. I should also mention that a python file cannot really handle configuration levels, like what you can do in a json or a ini file. Storing configurations for all plugins in one file could also be a bad idea, because the constant used by a plugin could have the same name as a constant from an other plugin, resoulting in conflicts (this type of issue can easely be resolved in a json or yaml file by requiring a namespace for each plugin).

If you create a module that handles default values, I don't see the advantages of using a python file instead of a json, yaml, ini file or whatever format you want.

[VForiel]

Using python as config files doesn't come from nowhere. Sphinx is using this method and it's proven to work.

The problem you mentioned

importing the file to read the configuration will miss default value if the user decide to empty the file

Ok, the user can empty everything without consideration of what he will break... the config using python is not the problem here.

there is also security concerns: if a user give his configuration file to another user, he can insert code that will be executed in the workspace of the second user, generally runing code in the configuration is a bad idea

Yes, the user can accept a configuration file that will run weird code... as he can accept a plugin that will run weird code, the user is supposed to look at what he put in his bot... whatever the type of config you use, as long as the user put something in his bot without verification, it's dangerous.

I should also mention that a python file cannot really handle configuration levels, like what you can do in a json or a ini file.

wdym? As a JSON file is strictely equivalent to a python dictionary, nothing can be done using JSON that cannot using python 🤔

Storing configurations for all plugins in one file could also be a bad idea, because the constant used by a plugin could have the same name as a constant from an other plugin, resoulting in conflicts (this type of issue can easely be resolved in a json or yaml file by requiring a namespace for each plugin).

Then it's more a question of conventions, it's also possible to apply the same type of convention using python files (ex: start every config variable with the name of the plugin, or put them in a class that have the name of the plugin ... or even using a unique dictionary for each plugin ! There is plenty of possible solutions). If two plugin share the same name (ie namespace), the problem is the same using json files.

The advantage of this solution

Using python file allow to deal with variables instead of dictionnary entries, which is way more conveniant. Moreover, it's easier to enderstand that "import config" will... import the configuration variables contained in the file than using a function defined somewhere and that return something... that we have to check the doc to deal with.

Moreover, running code in the configuration can have benefits. It's a file called at the very begining of the bot start and it is never overwritten, so it allow for users to add pre-run procedure to the bot, such I did in docs/conf.py (run by sphinx)

TL;DR

The bot is made to be user friendly, not to prevent user stupidity
Python files are more permissive, it mean that we can do a lot of things with it... including bad things. But the setup tool, the comments (which are not possible in JSON) etc. allow users to only edit what they understand

[Aeris1One]

Then it's more a question of conventions, it's also possible to apply the same type of convention using python files (ex: start every config variable with the name of the plugin, or put them in a class that have the name of the plugin ... or even using a unique dictionary for each plugin ! There is plenty of possible solutions). If two plugin share the same name (ie namespace), the problem is the same using json files.

Hum, actually, about conventions, maybe it would be better to enforce them, or else you'll be taking the risk of them not being respected. I would advocate restricting plugins to only seeing their own configuration, that would prevent a plugin trying to reinvent the wheel by reading itself some config which would otherwise be handled by the core.

Using python file allow to deal with variables instead of dictionnary entries, which is way more conveniant. Moreover, it's easier to enderstand that "import config" will... import the configuration variables contained in the file than using a function defined somewhere and that return something... that we have to check the doc to deal with.

Well, we can make a config.py which setup all those variables by reading the config. We would still need to import config and that would import the exact same variables, excepted that they would have been extracted from a JSON/YAML file. That way it would also be possible to dynamically edit this config file.
For example, upon plugin installation, some questions are asked to the user regarding the configuration to make and a namespace is added at the end of the configuration file.

e.g. for the friendly-ban plugin, upon installation you would be asked some questions like "which server would you like to enable friendly-ban in?" and then this may be added at the end of the config file:

- friendly-ban:
  - enabled-servers:
    - 125723125685026816
    - 835218602511958116
  - disabled-events:
    # this is empty by default, feel free to add any event id you want to disable

Actually, I'm not sure about the import config method which would import all config vars, maybe it would be better to import instead some functions (like config.get(enabled-servers) which would return a list of children, here they're servers IDs) and restrict a plugin to only seeing its config option. Zero-trust policy.

[VForiel]

Ok, tell me what you think about that:

The plugin config.py file content:

When this file is squashed in the main config.py file, it gives this:

In that way, we avoid name conflits by always using a class that have the name of the plugin (there is still a possible conflict in plugin names which will be tedious to remove)

Also, as the config.py file is always run when the bot start (and also during the setup), the plugin can add some configuration steps.

[ascpial]

Ok, the user can empty everything without consideration of what he will break... the config using python is not the problem here.

If the config is setup properly, there should be default values for settings that the user did not fill. This is not possible using the python file for the configuration directly, but if you setup a json-based system for example, the configuration reader could fill the empty values.
I should also mention that all the configuration files I saw allows you to only enter the values you want to edit.

Yes, the user can accept a configuration file that will run weird code... as he can accept a plugin that will run weird code, the user is supposed to look at what he put in his bot...

As Aeris1One said, we should be in a zero-trust policy: the user is more likely to edit the configuration file that install plugins that are not ship with the installer (I think we will make an installer to improve the user accessibility).

nothing can be done using JSON that cannot using python

It's much more intuitive to store nested data in JSON than in python.
I think users are going to edit the configuration file manually at some point. As you can see in the following example, the structure of the JSON code is much more intuitive than the nested classes you could use in Python. If you want to store data in a dictionary in the Python file, I don't see the point of using Python for this use case.

{
  "levels": {
    "default_levelup": {
      "type": "message",
      "content": "GG {user.mention}! You are now at the level {user.level}!",
      "delete_after": "5s",
    }
}

class Levels:
    class DefaultLevelup:
        type=discord.Message
        content="GG {user.mention}! You are now at the level {user.level}!"
        delete_after=datetime.timedelta(seconds=5)

I think using the types is the best way to do it for the Python configuration, as you don't need any extra parsing but the user needs to handle imports and to exactly know what he is doing.
If you add an extra layer of parsing, it would not be convenient for the developers, where they could just declare the type of data they are waiting for when loading the plugin or whatever, and also add a check for the type of data when the bot is loading, to prevent errors before they occurs.
If you are a new user, what type of configuration do you thing you will understand at first?

Then it's more a question of conventions, it's also possible to apply the same type of convention using python files (ex: start every config variable with the name of the plugin, or put them in a class that have the name of the plugin ...

Just imagine there is a plugin called archive_manager, and there is a sub option called archive_duration. The key for the configuration could quickly go very long?

or even using a unique dictionary for each plugin ! There is plenty of possible solutions). If two plugin share the same name (ie namespace), the problem is the same using json files.

As said before, what is the point of using a dictionnary where you could directly use a JSON file?

[VForiel]

Ok, j'ai fais une petite modification consistant à créer un fichier core/default_config.py qui contient les config de base du bot et auquel les config des autres plugins sont concaténés. Ce fichier n'est jamais exécuté et n'est importé que par le fichier config.py. De là, le fichier conf.py ne fait que redéfinir les variables que l'utilisateur souhaite modifier. Le fichier setup.py s'occupe à la fois de la concaténation des config dans core/default_config.py mais aussi de la création du fichier conf.py et l'ajout du token

Coté plugin, la config se définit via un fichier config.py contenant une classe ayant le même nom que le plugin:

Dans cette classe, le plugin peut définir tout ce qu'il souhaite (variables simples, dico, whatever)

De cette façon, la mise à jour du bot ou de ses plugins modifiera le fichier core/default_config.py mais pas le fichier conf.py ce qui rend les mises à jour transparent pour l'utilisateur (il n'aura jamais besoin de refaire son fichier de config)

[VForiel]

Aussi, cette classe peut définir des accesseurs custom sur ces attributs, ce qui permet de faire de la validation/protection des données

[theogiraudet]

Bon je viens également donner mon avis sur le type de config, et je suis assez d'accord avec @ascpial et @Aeris1One. Pour moi, aucun intérêt de faire de la config avec un langage de programmation, et cela pour plusieurs raisons :

• La config ne contient que de la donnée, aucune logique (sinon ce n'est pas de la config mais du scripting)
• La config est censée pouvoir être éditée par quelqu'un qui n'est pas programmeur -> plus le langage pour faire cette config est puissant, plus tu as de forme pour exprimer une même config et donc plus as de chances de perdre la personne qui tente d'installer le bot
• Dans la même ligné que le point précédent : plus le langage est puissant et plus tu as de chances d'exprimer quelque chose qui risque de faire planter le programme/qui a des effets de bord. Un langage plus restreint, c'est avoir un meilleur contrôle sur ce que saisi l'utilisateur. Pour pallier à cela en restant sur du Python, ça oblige a plus documenter en indiquant ce que l'utilisateur doit taper dans la config pour ne pas tout casser, ce qui ne serait pas nécessaire sur un langage comme YAML.
• Plus compliqué de vérifier la validité de la config. Une variable qui n'existe pas et étant définie par l'utilisateur n'est pas considérée comme une erreur et une variable qui aurait dû être définie mais qui ne l'est pas ne sera détectée que au runtime et non au lancement de l'application. Ça rend la chose plus difficile à déboguer pour un utilisateur donc
  -> Ce n'est pas pour rien que l'on fait de la config dans des langages spécifiques (DSL), plus le langage est restreint, moins il est risqué à utiliser

Si vous décidez tout de même de rester sur Python : ne JAMAIS faire un import comme ça de la config, ça pose de gros problèmes de sécurités (voulu par l'utilisateur, ou non). Au moins passez par le service de parsing de Python (module ast) pour s'assurer que l'import est safe.

[VForiel]

J'ai donc changé la gestion de la config pour utiliser des fichiers yaml. Il y a donc désmorais dans les plugins un fichier "config.yaml" et un fichier "setup.py" contenant une fonction "run()". Cette dernière est appelé lorsque le script setup.py est lancé et que l'utilisateur fait le choix de configurer le plugin en question.

Il y a donc :

• Un fichier core/default_config.yaml qui contient les paramètres de configuration (avec valeurs par défaut) du bot
• Des fichiers plugins/<plugin>/config.yaml
• Un fichier config.yaml qui est créé par setup.py et qui override les paramètres des autres fichiers (qui doit donc être le seul modifié par l'utilisateur ... bien qu'il n'ai pas besoin vu que le script de setup propose tout ce qui faut)
• Un fichier core/config.py permettant de load cette config ainsi que tous les fichiers plugins/<plugin>/config.yaml le tout au sein du même dictionnaire global_config. Pour être utilisé, il faut donc faire from core.config import global_config

[theogiraudet]

Quelques petits points à revoir mais sinon ça me semble pas mal !
Je suis cependant pas fan de cela :

Pour être utilisé, il faut donc faire from core.config import global_config

Pour moi, un plugin ne devrait pas pouvoir avoir accès à la config des autres plugins, d'autant plus si l'accès est en édition. Je pense que la config devrait être injectée à la classe principale du plugin.

[VForiel]

Pour moi, un plugin ne devrait pas pouvoir avoir accès à la config des autres plugins, d'autant plus si l'accès est en édition. Je pense que la config devrait être injectée à la classe principale du plugin.

Dans d'autres projets j'aurais été d'accord mais je doute que ça soit nécessaire ici... et je ne vois pas trop comment faire ça proprement 😅

Le fait d'y avoir accès en lecture peut être intéressant, mais en effet, en écriture ça ne devrait pas arriver.

[theogiraudet]

et je ne vois pas trop comment faire ça proprement 😅

Une classe s'occupe de charger les plugins (appelons-la PluginLoader ou PluginManager). Celui-ci est le seul à avoir accès à l'ensemble de la config. Au chargement du plugin (appel d'une fonction spéficique au plugin), il passe à celui-ci la config spécifique au dit plugin.

[ascpial]

Je suis tout à fait d'accord, et ca pourra aussi permettre de charger des données liées à l'extension sans lancer le bot (par exemple vérifier la configuration)

[ascpial]

J'ai relu les changements et testé les nouvelles fonctionnalités et tout fonctionne comme attendu !

[ascpial]

Mettre à jour le README pour indiquer comment cloner les plugins de Gunivers

[VForiel]

Le merge de cette PR est trop complexe du à l'arrivée du submodule, le merge s'est donc bricolé de cette façon :

• Branche new_structure (cette PR) renommée en beta et mise en branche par défaut
• Branche beta renommée en dev, les changements fais là bas devront être ramenés sur cette branche.

(cette PR peut donc être considéré comme merged)