-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Incompatible config file for new major release acceptable? #66
Comments
I actually prefer rather slim configuration files, where no or very few settings have to be defined. We could implement a 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 what do you think? |
Good proposal, that is clearly combines all advantages (old config files still work, no need to query existence). I vote for this one. |
I also vote for this. Nice idea. |
I also |
I have added a PullRequest for a seperate defaultConfig-File, which is merged with the user config (PR #67) |
In my opinion, using JSON files only improves the possibility of validation. Your suggestion would work for new installations, but affect existing ones. 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. |
"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. |
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 😃 |
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. |
Hey guys, i would suggest to stay with the js config and try to make the update as smooth as possible. 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 👍 |
@LukeSkywalker92 TeleFrame is a great project! 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. 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. 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 😏 |
I had a look at @gegu implementation in feature/default-config and have one comment: 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. |
The default values for the phrases will be removed in feature/internationalization - fce6959. And yes, the other default values should be removed as well. |
for features in the future , i think the JSON file is a better choice. 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 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. |
We can almost always find a way to the solution. Only the question of effort and benefit arises. Without thinking much, for example:
There are certainly even more approaches to this problem.
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.
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. |
of course you will always find a solution. The question is: Why adopt a complicated approach when we can adopt a simple one...
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:
The install-Script will copy config.example.* to config.* and the teleframe is ready. 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.
You are right, but the user was already able to configurate it! Why he is not able to do it again? |
Yes, generated only for reference - but ready to use. config.example.* removed from git.
Because it's annoying 😄 And again, editing JSON files manually is more error-prone if you want to try something out. |
Maybe we should go one step further with our thoughts.
|
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,...) |
This of course changes the initial situation.
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 |
I have changed my PR (#67). 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. |
I think it's a good idea if we have a separate branch for changing the configuration. If nobody has a veto, I'd like to include the outsourcing of the texts from feature/internationalization before we go any further. |
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). |
In default-i18n-config, I created a branch for changing the default configuration and implementing internationalization. At the first start the In the configuration object /**
* 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. |
For me it is excellent. Very nice work. Some notes:
|
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.
You are right, the installer does not create any other entries anymore.
Yeah, that's one of the many things left to do.
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 Please adjust your PR's to use the defaultConfig.js and config.examplse.json. Merry Christmas, everybody. |
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:
I think we should find an agreed way forward how to deal with this.
The text was updated successfully, but these errors were encountered: