12
\$\begingroup\$

I'm working on a simple dictionary tool, with a base class that can be extended by plugins to represent different dictionaries. The plugins are organized in the filesystem like this:

plugins/
├── dict1
│   ├── ...
│   └── dict1.py
└── dict2
    ├── ...
    └── dict2.py

Each plugin defines a class named Dictionary, which happens to extend a BaseDictionary, in case that's relevant.

I implemented discovering and loading the plugins like this:

import os
from imp import find_module, load_module

BASE_DIR = os.path.dirname(__file__)
PLUGINS_PATH = os.path.join(BASE_DIR, 'plugins')


def discover_dictionaries():
    import plugins
    for plugin_name in os.listdir(PLUGINS_PATH):
        plugin_path = os.path.join(PLUGINS_PATH, plugin_name, plugin_name + '.py')
        if os.path.isfile(plugin_path):
            try:
                fp, pathname, description = find_module(plugin_name, plugins.__path__)
                m1 = load_module(plugin_name, fp, pathname, description)
                fp, pathname, description = find_module(plugin_name, m1.__path__)
                m2 = load_module(plugin_name, fp, pathname, description)
                class_ = getattr(m2, 'Dictionary')
                yield plugin_name, class_()
            except ImportError:
                print('Error: could not import Dictionary from {0}'.format(plugin_path))

This works, but it's kind of ugly, and probably I'm doing it all wrong. How to do this the "right way," or at least better?

If you want to play with the code to see if your refactoring ideas work, clone the project from GitHub, and run this as a sanity check:

# should output: dummy
python -c 'import util; print util.discover_dictionaries().next()[0]'
\$\endgroup\$
4
\$\begingroup\$

I started a project with a structure similar to the one you detailed:

Project structure

My first order of business was to move the import plugins out of the discover_dictionaries() function and to the top of the file.

My second order of business was to create an iterator which would iterate over the discover_dictionaries() generator and print what it had found:

for next_plugin in discover_dictionaries():
    print next_plugin

My third order of business was to create empty classes in each of the dict* paths, which looked similar to:

import os

class Dictionary:
    def __init__(self):
        pass

Following that, I added a print to the generator to see what it saw:

def discover_dictionaries():
    for plugin_name in os.listdir(PLUGINS_PATH):
        plugin_path = os.path.join(PLUGINS_PATH, plugin_name, plugin_name + '.py')
        print plugin_path

This gave me the following output:

C:/Users/jsanc623/PycharmProjects/AutoloadModules\plugins\dict1\dict1.py
C:/Users/jsanc623/PycharmProjects/AutoloadModules\plugins\dict2\dict2.py
C:/Users/jsanc623/PycharmProjects/AutoloadModules\plugins\dict3\dict3.py
C:/Users/jsanc623/PycharmProjects/AutoloadModules\plugins\__init__.py\__init__.py.py
C:/Users/jsanc623/PycharmProjects/AutoloadModules\plugins\__init__.pyc\__init__.pyc.py

If I put the same print statement inside the if os.path.isfile(plugin_path): block, the output was:

('dict1', <dict1.Dictionary instance at 0x0000000001FABA48>)
('dict2', <dict2.Dictionary instance at 0x0000000001FB04C8>)
('dict3', <dict3.Dictionary instance at 0x0000000001FB0F08>)

So, it does work. Now, let's see how we can clean it up.

The first thing I did was to get rid of BASE_DIR:

BASE_DIR = os.path.dirname(__file__)
PLUGINS_PATH = os.path.join(BASE_DIR, 'plugins')

as follows:

PLUGINS_PATH = os.path.join(os.path.dirname(__file__), 'plugins')

Next, we can get rid of class_ by replacing the yield statement from:

class_ = getattr(m2, 'Dictionary')
yield plugin_name, class_()

To a much simpler:

yield plugin_name, getattr(m2, 'Dictionary')()

Next, I added an if statement to return if it encountered a 'pyc' or 'init' file:

for plugin_name in os.listdir(PLUGINS_PATH):
    if 'pyc' in plugin_name or 'init' in plugin_name:
        return
    else:

We can also get rid of m1, by casting pathname:

fp, pathname, description = find_module(plugin_name, plugins.__path__)
fp, pathname, description = find_module(plugin_name, [pathname])
m2 = load_module(plugin_name, fp, pathname, description)

But, we can get even sneakier and get rid of the assignment on the first find_module by doing so:

fp, pt, dc = find_module(plugin_name, [find_module(plugin_name, plugins.__path__)[1]])
m2 = load_module(plugin_name, fp, pt, dc)

Which, granted makes our lines a little long. However, let's get a little crazier with our yield:

yield plugin_name, getattr(load_module(plugin_name, fp, pt, dc), 'Dictionary')()

So, in the end, the original would now look like:

import os
from imp import find_module, load_module
import plugins

PLUGINS_PATH = os.path.join(os.path.dirname(__file__), 'plugins')

def discover_dictionaries():
    for plugin_name in os.listdir(PLUGINS_PATH):
        if 'pyc' in plugin_name or 'init' in plugin_name:
            return
        else:
            plugin_path = os.path.join(PLUGINS_PATH, plugin_name, plugin_name + '.py')
            if os.path.isfile(plugin_path):
                try:
                    f, p, d = find_module(plugin_name, [find_module(plugin_name, plugins.__path__)[1]])
                    yield plugin_name, getattr(load_module(plugin_name, f, p, d), 'Dictionary')()
                except ImportError:
                    print('Error: could not import Dictionary from {0}'.format(plugin_path))

And it would still function. If you want to make it a bit more concise at the cost of iterating over 'pyc' and 'init' files, you can omit the if/else statement:

import os
from imp import find_module, load_module
import plugins

PLUGINS_PATH = os.path.join(os.path.dirname(__file__), 'plugins')

def discover_dictionaries():
    for plugin_name in os.listdir(PLUGINS_PATH):
        plugin_path = os.path.join(PLUGINS_PATH, plugin_name, plugin_name + '.py')
        if os.path.isfile(plugin_path):
            try:
                f, p, d = find_module(plugin_name, [find_module(plugin_name, plugins.__path__)[1]])
                yield plugin_name, getattr(load_module(plugin_name, f, p, d), 'Dictionary')()
            except ImportError:
                print('Error: could not import Dictionary from {0}'.format(plugin_path))

Readability out of the window, but we save assignments?

\$\endgroup\$
4
\$\begingroup\$

Since you seem to be using Python 3, you could use the new pathlib module from Python 3.4 to have a Path object. Since paths become real objects instead of mere strings, this module improves the safety of path-handling. It is somehow simpler to read than the old os.path and is more expressive.

Here is your code rewritten using the pathlib module (not tested though):

from pathlib import Path
from imp import find_module, load_module

BASE_DIR = Path(__file__).parent
PLUGINS_PATH = BASE_DIR / 'plugins'


def discover_dictionaries():
    import plugins
    for plugin in PLUGINS_PATH.iterdir():
        name = plugin.name
        plugin = plugin / (name + '.py')
        if plugin.is_file():
            try:
                fp, pathname, description = find_module(name, plugins.__path__)
                m1 = load_module(name, fp, pathname, description)
                fp, pathname, description = find_module(name, m1.__path__)
                m2 = load_module(name, fp, pathname, description)
                class_ = getattr(m2, 'Dictionary')
                yield name, class_()
            except ImportError:
                print('Error: could not import Dictionary from {0}'.format(str(plugin)))
\$\endgroup\$
  • \$\begingroup\$ Thanks, that's good to know! I'm trying to stay prepared for migrating to Python 3 someday, hence the print(). The server where I want to deploy has only Python 2, but maybe I can make it work with Python 3 and make this leap. My biggest concern here was not so much constructing paths, as the module loading part. \$\endgroup\$ – janos Aug 13 '14 at 21:36
  • 1
    \$\begingroup\$ @janos The module machinery has also been rewritten in Python 3.3 or 3.4 (I don't remember) and the one you use has been deprecated. However, I am no import hook expert. You should have a look at importlib :) \$\endgroup\$ – Morwenn Aug 13 '14 at 21:37
3
\$\begingroup\$

After several more tries, I came up with a solution with some advantages:

  • slightly simpler, using __import__ instead of the imp package
  • no need to import plugins, I can import plugins.somename.name directly using __import__
  • removed several intermediary variables and assignments

Implementation:

import os


PLUGINS_DIR = 'plugins'
PLUGINS_PATH = os.path.join(os.path.dirname(__file__), PLUGINS_DIR)


def discover_dictionaries():
    for plugin_name in os.listdir(PLUGINS_PATH):
        plugin_path = os.path.join(PLUGINS_PATH, plugin_name, plugin_name + '.py')
        if os.path.isfile(plugin_path):
            try:
                name = '.'.join([PLUGINS_DIR, plugin_name, plugin_name])
                module = __import__(name, fromlist=['Dictionary'])
                yield plugin_name, getattr(module, 'Dictionary')()
            except ImportError:
                print('Error: could not import Dictionary from {0}'.format(plugin_path))

Against the advice of @jsanc623, I decided to keep intermediary variables like name and module, as I think it's more readable this way, with shorter lines.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.