Skip to content
This repository has been archived by the owner on Feb 2, 2022. It is now read-only.

Make team_size configurable in config.json #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Make team_size configurable in config.json #8

wants to merge 1 commit into from

Conversation

zoidyzoidzoid
Copy link
Contributor

Closes #7

The default is set in 'main()' and sent to the GatherBot model, but
maybe I should be setting the default all the way through.

Closes #7

The default is set in 'main()' and sent to the GatherBot model, but
maybe I should be setting the default all the way through.
@veryhappythings
Copy link
Owner

I am torn between this and having a Config class that could be used anywhere to grab data. I don't think it matters much here, but dragging data through multiple classes like this feels like it isn't the smoothest way to do things.

@zoidyzoidzoid
Copy link
Contributor Author

Yeah, let me rethink this. It does feel dirty.

@veryhappythings
Copy link
Owner

Loading a file everywhere, or loading it once and saving that state in memory, also feels kinda dirty. I've not hit on a good answer for it :-/ Docker's driven everyone into using environment variables - I suppose that might be an option?

@veryhappythings
Copy link
Owner

veryhappythings commented Aug 18, 2016

Having spent a bit of time working on some docker stuff yesterday, and sleeping on this, I do like the idea of at least having environment variables as an option. I reckon a Configuration class that can pull from a dict (currently coming in from json) and environment variables could be really nice. Something like

import os


class Configuration:
    def __init__(self):
        with open('config.json') as f:
            self.config = json.load(f)

        for key in ['token', 'team_size']:
            env_key = ''.join('DG_', key.upper)
            if env_key in os.environ:
                self.config[key] = os.environ[env_key]

I'm not happy with how I'm getting the file name here, but, errr, I guess we could pass that in or cache the dict or something. Or maybe just abandon it and go environment variable only.

If I'm overthinking this, it's because I'm in the middle of a big refactoring project at work so I've been thinking about architecture of tiny things a lot :)

@zoidyzoidzoid
Copy link
Contributor Author

Nah, your idea sounds great.

Will work a bit more on it when I get a chance.

@veryhappythings
Copy link
Owner

Oh, hi! I've changed a lot since this PR started - hopefully the code still makes some sense :) I still don't have one good answer for how we should add config like this, but environment variables still suit the way I deploy it through docker. There's now one clear path through the code, from main into discord_gather and on to gatherbot, which now knows nothing about Discord, so I guess config can comfortably flow through too.

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

Successfully merging this pull request may close these issues.

2 participants