Skip to content
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

Extract settings to external configuration file #49

Merged
merged 5 commits into from
Mar 15, 2015
Merged

Extract settings to external configuration file #49

merged 5 commits into from
Mar 15, 2015

Conversation

aaparella
Copy link
Contributor

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
Copy link

jdm commented Nov 21, 2014

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link

jdm commented Nov 21, 2014

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
Copy link
Contributor Author

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 shaunagm merged commit b98dac4 into shaunagm:master Mar 15, 2015
@shaunagm
Copy link
Owner

Sorry to take forever on this. I finally merged your changes: a6bd895

Haven't addressed including the name in the message, I've made a separate issue:
#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants