shaunagm / WelcomeBot

Other
35 stars 43 forks source link

Extract settings to external configuration file #49

Closed aaparella closed 9 years ago

aaparella commented 9 years ago

Use configparser library to parse settings from 'settings.ini' file. Works as is, but not entirely sure what settings we would want to make parametrizable. Default values are left in code for now. Can easily be expanded, and structure of settings file is up for discussion (very simple layout used for the time being). Any additional settings can simply be added using the syntax shown in the settings.ini file.

jdm commented 9 years ago

One of the standard ways of doing this is to return the config dictionary from the parsing function and pass it to the functions that would make use of configurable values. Also, instead of leaving default values in the code, it would make more sense to move those all into a settings.ini.default, and modify the setup instructions for the bot to include copying that file to settings.ini and making local changes.

aaparella commented 9 years ago

That was definitely the plan, I was just looking for a quick proof of concept that fit into the existing code base (which uses global variables for configuration settings). I'll refactor it in that manner soon (if not today (if not right now)).

shaunagm commented 9 years ago

Just as a heads up, we have an event tomorrow (Saturday) where students may be working on WelcomeBot. So if any refactoring is going to happen, it should happen today, or sometime after Saturday, so not to introduce unnecessary merge conflicts into newcomers' first PRs.

It's also worth stating that we want this code to be readable for relatively inexperienced programmers. So the choices we make should prioritize human comprehension. In fact, my plan over the next month or so is to create tutorial-style documentation for WelcomeBot, as seen in this issue. This change should come with tutorial-documentation. You certainly don't have to write it, @aaparella, but I may hold off on merging the change until someone does. I can get to it myself within the next few weeks.

aaparella commented 9 years ago

The settings are now passed around as a dictionary when needed, instead of using global variables as before.

I've added everything except for the help / hello lists to the settings parser. The syntax can largely be deduced from the code that is already there, so perhaps a starter task for tomorrow could be adding the two remaining configuration options?

jdm commented 9 years ago

Thanks for doing this @aaparella :) The initial greeting message would also be good to make configurable, which could be a task for tomorrow as well.

aaparella commented 9 years ago

I've gone ahead and added the creation of a welcome_message string for the Bot, but I'm not sure how we would want to handle the inclusion of the greeted user, etc., if at all? That seems difficult to configure in a settings file, and would essentially require the person writing it to know python already.

shaunagm commented 9 years ago

Sorry to take forever on this. I finally merged your changes: https://github.com/shaunagm/WelcomeBot/commit/a6bd89512f9763e35582887c50b812ce1fe2e3ad

Haven't addressed including the name in the message, I've made a separate issue: https://github.com/shaunagm/WelcomeBot/issues/53

I also channed your imports from a settings.ini that's parsed using a configparser to a set of variables in a .py module that are imported in. The way variables are passed around bot.py right now is currently a mess (my fault!) so having the variables available globally via an import makes thing simpler.