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

Default "extra" config variable omits all exclude/enable module settings #202

Open
ghost opened this issue Aug 9, 2015 · 3 comments
Open

Comments

@ghost
Copy link

ghost commented Aug 9, 2015

file: bot.py

if not hasattr(self.config, 'enable'):
    for fn in os.listdir(os.path.join(os.getcwd(), 'modules')):
        if fn.endswith('.py') and not fn.startswith('_'):
            filenames.append(os.path.join(home, 'modules', fn))
else:
    for fn in self.config.enable:
        filenames.append(os.path.join(home, 'modules', fn + '.py'))

if hasattr(self.config, 'extra'):
    for fn in self.config.extra:
        if os.path.isfile(fn):
            filenames.append(fn)
        elif os.path.isdir(fn):
            for n in os.listdir(fn):
                if n.endswith('.py') and not n.startswith('_'):
                    filenames.append(os.path.join(fn, n))

The problem is that, by default, extra includes the same module folder that the first for-loop crawls. And since there's no check if a module is on the exclude/enable list during the second for-loop it just adds them anyway.

@kaneda
Copy link
Contributor

kaneda commented Aug 9, 2015

I think this requires a bit more investigation.

@ghost
Copy link
Author

ghost commented Aug 9, 2015

Does this look good?
I was also thinking of making the latest filename of the same name overwrite the array netry instead of duplicating it, does that sound good?

    def setup(self):
        self.variables = {}

        filenames = []

        module_folders = [os.path.join(home, 'modules')]
        module_folders.extend(getattr(self.config, 'extra', []))
        modules = []

        excluded = getattr(self.config, 'exclude', [])
        enabled = getattr(self.config, 'enable', [])

        for folder in module_folders:
            if os.path.isfile(folder):
                filenames.append(folder)
            elif os.path.isdir(folder):
                for fn in os.listdir(folder):
                    if fn.endswith('.py') and not fn.startswith('_'):
                        name = os.path.basename(fn)[:-3]
                        if name in enabled or not enabled and name not in excluded:
                            filenames.append(os.path.join(folder, fn))

        for filename in filenames:
            name = os.path.basename(filename)[:-3]
            try: module = imp.load_source(name, filename)
            except Exception, e:
                print >> sys.stderr, "Error loading %s: %s (in bot.py)" % (name, e)
            else:
                if hasattr(module, 'setup'):
                    module.setup(self)
                self.register(vars(module))
                modules.append(name)

        if modules:
            print >> sys.stderr, 'Registered modules:', ', '.join(sorted(modules))
        else:
            print >> sys.stderr, "Warning: Couldn't find any modules"

        self.bind_commands()

PS: Sorry for just pasting code in here instead of making pull requests. I'll do that sometime tomorrow.

@kaneda
Copy link
Contributor

kaneda commented Aug 9, 2015

@Bekey I think the else with the is a weird thing to do. I'll need to take a closer look once you open a PR.

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

No branches or pull requests

1 participant