Incompatible config file for new major release acceptable?

[mistau]

For new features we need additional config options making this incompatible with an old config file. In one of my patch I decided to query the existence of the according entry to make sure I stay compatible with the old version. This generates extra code which has to be tested though, in my opinion this is not a viable path forward.

How do we want to deal with that situation? Options I would see is:

1. query the existence of the new config variable and default to old behavior
2. accept incompatible config files for new major releases
3. option (2) + provide an update script adding default entries for the new options

I think we should find an agreed way forward how to deal with this.

[gegu]

I actually prefer rather slim configuration files, where no or very few settings have to be defined. The user is not overwhelmed by too many settings he has to make.

We could implement a defaultConfig.js, which preconfigures all options necessary for execution with meaningful values. This is merged with the user configuration.

For example

const config = require('./js/defaultConfig');
Object.assign(config, require('./config/config'));

However, it would then be necessary to document the configuration options in the config.example.js.

what do you think?

[mistau]

Good proposal, that is clearly combines all advantages (old config files still work, no need to query existence).

I vote for this one.

[LukeSkywalker92]

I also vote for this. Nice idea.

[sohamakl]

I also

[LuHKae]

I have added a PullRequest for a seperate defaultConfig-File, which is merged with the user config (PR #67)

[gegu]

In my opinion, using JSON files only improves the possibility of validation. If necessary, we could also perform the check with JSON.stringify/JSON.parse.

Your suggestion would work for new installations, but affect existing ones. In this case we would also have to write an update script.

Manual editing of JSON files in an editor is also more difficult. You are forced to use quotation marks on the keys and no comments can be used in the file.

Our goal should be to deliver a configuration where the user needs to change as little as possible and future extensions will not cause any problems.

[LuHKae]

"Our goal should be to deliver a configuration where the user needs to change as little as possible and future extensions will not cause any problems." - My Solution offers exactly that. The smalest possible userconfig is {"botToken":"XYZ"} and new config-params will be updated threw the default-config.

Whether JSON or JS-Object is the better solution, is in my opinion a matter of taste. there's no right or wrong, we just have to find a compromise.

[gegu]

That's right, your solution doesn't do anything other than what I suggested. However, it requires additional conversion of existing configurations.

More error-prone is the manual editing of a JSON file without tools.

Don't you find it easier for the end user to have a documented configuration file where comments are simply set/removed to enable/disable features?

I am also open to all approaches 😃

[mistau]

I don't see a fundamental advantage using one or the other format - apart from the compatibility argument which would be a reason to stick with our js config files.

From my perspective it's key that we conclude here, the other patches requiring a config option are somehow depending on this decision.

[LukeSkywalker92]

Hey guys,

i would suggest to stay with the js config and try to make the update as smooth as possible. As suggested, we should have default values for all parameters. I would prefer to place this default.js file in the js folder to avoid accidental mistakes when users edit the config. I think @gegu already suggested this.

When we're moving to a new config, maybe it will be possible to have a language option, that sets the defaults for all strings in the config file. Then the user just need to set 'en' or 'de' for example. If the preferred language is not availiable, one can set the strings in the config.

P.s.: Thank you very much for your contributions. I love to see that this project is pushed forward. Even if I don't find enough time at the moment. Thumps up :+1:

[gegu]

@LukeSkywalker92 TeleFrame is a great project! It is fun to join in and has a lot of potential for new extensions.

In feature/default-config - I have started the preparation for the implementation of the standard configuration. Please check if it's okay for everyone and what we could improve.

In my view, it would be good if we did not have to do the documentation several times. What do you think about commenting on defaultConfig.js and copying it to config/config.example.js using npm update script during installation/update? Maybe we could remove the configuration documentation from README.md and refer to the commented default configuration.

Only a minimal configuration file should be created for future installations. It would suffice as @LuHKae has suggested the botToken and possibly an empty array for the whitelist.

I've also been working on the text configuration feature recently - feature/internationalization. If I didn't miss anything, the requirements should be met. There is also the possibility for the user to create the personal file config/texts.js. If no language setting is defined, the system language is determined and loaded if a language file is available. This would be basically done and you could take a look at it.

Not to affect anyone, I wouldn't merge this into develop after the defaultConfig and until the pull requests #53, #60, and #64 are integrated.

Finally, a personal note: Since my grammar is terrible when writing English texts, a translation tool does it for me. For safety's sake, I apologize in advance if a statement should sound too stiff 😏

[mistau]

I had a look at @gegu implementation in feature/default-config and have one comment: The code in js/renderer.js still is prepared for the case that config variables are not defined. e.g. text.innerHTML = config.voiceReply.recordingError || "Voice message has failed!";

While this doesn't harm I propose to stay consistent and remove the extra code. With the "default config" approach we ensure that all variables are defined, there is no need to check this whenever we use a config variable. Apart from that rather cosmetic change i propose to merge this into develop.

[gegu]

The code in js/renderer.js still is prepared for the case that config variables are not defined. e.g. text.innerHTML = config.voiceReply.recordingError || "Voice message has failed!";

The default values for the phrases will be removed in feature/internationalization - fce6959f6ad6b2f4ed62ef3cce5688ac6f93af5f.

And yes, the other default values should be removed as well.

[LuHKae]

for features in the future , i think the JSON file is a better choice. for example: if you want to configure the bot by sending messages to the bot, you have to be able to modify the configuration from the program. if the config is in a json, you can simply modify and save the file.

if the config is in a js-object embedded in a js-file, i spontaneously have no idea how to change the config. what are your opinions about this? do you have any ideas?

We should make sure that the "new" user-config is kept as small as possible. Otherwise the userconfig will not be modified, if a default value is changed. in my opinion, only values that differ from the default should really be added to the user-config.

in the end i don't care about the decision whether json or js. in my eyes we should only take care to be well positioned for the future and should not make our decision dependent on whether the user has to do the configuration again. we should separate from "old burdens" as early as possible and accept incompatibilities for new major releases.

@gegu my grammar is also bad, so i use a tool as well... i also apologize any bad sentence.

[gegu]

We can almost always find a way to the solution. Only the question of effort and benefit arises.

Without thinking much, for example:

• We could write to an additional config.botchagned.json file, which is the last to be merged.
• We implement a parser that changes or inserts the key/value directly in the config.

There are certainly even more approaches to this problem.

We should make sure that the "new" user-config is kept as small as possible.

Yeah, you're absolutely right.

Therefore my suggestion to write only bottoken and possibly the empty whiltelist in the config.js for new installations.

The defaultConfig.js could be copied to config.example.js when updating so that the end user has a documented configuration at hand. There we can even update their bottoken and the whitelist using an update script if we want. Then the user's config.example.conf can simply be copied after an update if he wants.

... and should not make our decision dependent on whether the user has to do the configuration again.

I don't see it that way. We should already try to design the configuration in such a way that someone without pronounced knowledge can cope with it.

Anyway, so far I don't see a showstopper for future enhancement in all our approaches.

[LuHKae]

of course you will always find a solution. The question is: Why adopt a complicated approach when we can adopt a simple one... And i think merging out of more than two files is not realy verifiable for the user.

Therefore my suggestion to write only bottoken and possibly the empty whiltelist in the config.js for new installations.

The defaultConfig.js could be copied to config.example.js when updating so that the end user has a documented configuration at hand. There we can even update their bottoken and the whitelist using an update script if we want. Then the user's config.example.conf can simply be copied after an update if he wants.

I don't understand your approach. How is the Workflow for an Update? Does the user copy the file config.examples.js to config.js and reconfigure all his changes? Or is the file config.examples.js only a reference for possible values?

I think there should be two files in git:

• defaultConfig.* (with well documented Key/Values)
• config.example.* (with only bottoken, whiltelist and Key/Values a normal user may will change)

The install-Script will copy config.example. to config. and the teleframe is ready. if the user want to change something, he will do it in config.*

On an update, the new defaultConfig.* will come frome GIT. The User-Config is not touched, so the bot is working with the new features, but with the changes the user really has mad.

If on install/update the config.example. is copied to config. we have to add an update script, which will generate an merged config-file. with the thought of finding the simplest possible solution, we should not suggest that.

I don't see it that way. We should already try to design the configuration in such a way that someone without pronounced knowledge can cope with it.

You are right, but the user was already able to configurate it! Why he is not able to do it again? the whole thing is a one-time change. in my eyes, you can trust the user to do the configuration again, if he gets the numerous great features in return.

[gegu]

Or is the file config.examples.js only a reference for possible values?

Yes, generated only for reference - but ready to use. config.example.* removed from git. During the installation we don't need a config to copy any more, This can be created by the script.

You are right, but the user was already able to configurate it! Why he is not able to do it again?

Because it's annoying 😄 And again, editing JSON files manually is more error-prone if you want to try something out.

[LukeSkywalker92]

Maybe we should go one step further with our thoughts. I think the long term goal should be a texteditor free configuration. Therefore an easy way of loading and saving the configuration file is important (point for json). What is also needed is to check the entered values for correctness. E.g. we can check for a valid bot token, data types (string, int, bool etc.) and so on (I think this is also a point for json, right? correct me if I'm wrong). So we have two points for json, but we need some part in the software that does all this checks and handles config load and save. So we need a js file. There also all the default values could go... Sure we will have some work for the next major update, but I think with this design we are well prepared for the future. So my summed up suggestion:

• a json file where the settings are stored (minimal style, only values that differ from default values)
• a js file that acts as an interface between the software and the json file
• long term goal: no texteditor needed for configuration (configuration via telegram or admin page in the teleframe software)

[Trenar]

IMHO, a minimal config.js is the best one can do. Given a good documentation of the teleframe software and all of its features / config params, it is the most user friendly way to have a default config and only change specific parameters. Sounds like it can be realized best with json in the js folder and a config.js that is merged with the default params on teleframe startup (or file change,...)

[gegu]

I think the long term goal should be a texteditor free configuration.

This of course changes the initial situation. If the user gets an interface and doesn't have to change anything manually, JSON is definitely preferable. My reservations related to manual editing.

What is also needed is to check the entered values for correctness. E.g. we can check for a valid bot token, data types (string, int, bool etc.) and so on (I think this is also a point for json, right?

Yes, absolutely. If we implement a JSON schema, it can be validated quite easily. Some modules are available for this.

In my opinion, we should coordinate the next steps a little so that not everyone starts to implement the same thing

[LuHKae]

I have changed my PR (#67). Now the user-config is a json-file and the default-config is a well documented js-file.

perhaps we should merge these PR to an extra feature branch. Then we are able to merge each feature (like JSON schema, config via bot-command, new install script ...) first to this feature branch and merge it to develop if we are really ready.

please let me know your opinion.

[gegu]

I think it's a good idea if we have a separate branch for changing the configuration. On the other hand, the open PR can be checked and adjusted again.

If nobody has a veto, I'd like to include the outsourcing of the texts from feature/internationalization before we go any further.

[mistau]

I am wondering what the advantage is of putting the #67 into a separate branch vs. merging it into develop. Most of the current pull requests heavily depend on the config part as they introduce new entries there. As we are not planning to introduce yet another config file variant there will be nothing on develop though, I therefore propose to use it right away and merge #67 into develop soon.

I would only see the need for a separate branch if we seriously plan to have another major release not yet integrating #67. (I cannot see what new feature would be in there).

[gegu]

In default-i18n-config, I created a branch for changing the default configuration and implementing internationalization.

At the first start the config/config.js is converted to config/config.json and a backup is created.

In the configuration object config the members getDefaults() and writeConfig() are available.

/**
 * Returns the default configuration object
 * @return {Object} default Configuration
 */
config.getDefaults()

/**
 * Write from defaultConfig deviating options to the config file
 * Notice: Arrays are always copied completely
 */
config.writeConfig()

Please see if all can live with it. I would merge the branch with develop and then the open PR if there are no conflicts.

Some things like README etc. still need to be adjusted. We can do this later. We should be able to continue with that now.

[LuHKae]

For me it is excellent. Very nice work.

Some notes:

• In my opinion, the file config\config.example.json should be as small as possible. If the user sets every possible key by cp the example-file, no changes of the default-config will be applied. I think only these keys should be set in config.example.json : botToken, whitelistChats, language
• The Language should be requestet in the install-script
• The 'language'-Key is missing in the default config

[gegu]

Thanks for the flowers 😃

While I was at it, I tried to implement the really needed things to handle the configuration. So we can concentrate on implementing the new features.

In my opinion, the file config\config.example.json should be as small as possible. If the user sets every possible key by cp the example-file, no changes of the default-config will be applied. I think only these keys should be set in config.example.json : botToken, whitelistChats, language

You are right, the installer does not create any other entries anymore. We can easily solve the problem with the default settings by calling writeConfig when loading the user configuration. Only settings that differ from the defaultConfig are taken into account. In this case we have to disable the watcher of pm2 for config.json. But we have to do that anyway, as soon as we implement an interface for the configuration.

The Language should be requestet in the install-script

Yeah, that's one of the many things left to do. I will make a list of what I have noticed, soon.

The 'language'-Key is missing in the default config

Right, I forgot to document this in the defaultConfig. I have made up for it.

var defaultConfig = {
  ...
  // Define the language to use.
  // If the language is not defined, the system language is loaded, if available.
  // If no language file could be determined, English is used by default.
  // language: "en",
  ...
}

I have merged into develop'.

Please adjust your PR's to use the defaultConfig.js and config.examplse.json. What is ready, I will soon mergeit.

Merry Christmas, everybody.