launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/enum-fail into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/enum-fail/+merge/104365

Gavin's version of enums.js only contains the pserv's enums, and twice, rather than the maasserver enums plus the pserv enums.  The reason seems to be listification of a generator expression: every load_module was loading “enum” and replacing the previous one.  So when we then iterate over the list of returned module objects, at least in Gavin's setup, we get the same module every time.  This seems to be a problem when the generator expression for the enum modules gets evaluated before we start processing modules.

(Gavin found all this stuff out, not me.)

It turns out imp.find_module's specification has changed from python 2.7.2 to 2.7.3: it now accepts module names with dots in them, which the 2.7.2 docs say it doesn't, but also we're hitting this problem.  Gavin and I both seem to be on 2.7.3, and yet our systems behave differently.

In this branch I get rid of find_module altogether and use just load_module, which seems to accept nested module names in both 2.7.2 and 2.7.3.  It gets a full nested module name, so that the imported modules don't clash.  By and large, perhaps apart from some extra parameter construction, the code is the cleaner and simpler for it.  It also no longer has an exception path for a perfectly normal and valid case (where a directory does not contain an enum.py).

For extra points, I'm now sorting the directory list.  The documentation for os.listdir doesn't promise consistent ordering, especially across different filesystems, and we want consistent ordering.

Jeroen
-- 
https://code.launchpad.net/~jtv/maas/enum-fail/+merge/104365
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/enum-fail into lp:maas.

=== modified file 'utilities/convert-enums.py'
--- utilities/convert-enums.py	2012-05-02 05:14:36 +0000
+++ utilities/convert-enums.py	2012-05-02 12:08:19 +0000
@@ -25,8 +25,8 @@
 from argparse import ArgumentParser
 from datetime import datetime
 from imp import (
-    find_module,
     load_module,
+    PY_SOURCE,
     )
 import json
 import os.path
@@ -65,13 +65,13 @@
     :param name: Name of module to load.
     :return: The imported module, or None if it was not found.
     """
-    path = os.path.join(src_path, package)
-    try:
-        found_module = find_module(name, [path])
-    except ImportError:
-        # No enum module here.  Ignore this package or directory.
+    path = os.path.join(src_path, package, "%s.py" % name)
+    if not os.path.isfile(path):
         return None
-    return load_module(name, *found_module)
+    full_name = '.'.join([package, name])
+    description = ('.py', 'r', PY_SOURCE)
+    with open(path) as module_file:
+        return load_module(full_name, module_file, path, description)
 
 
 def find_enum_modules(src_path):
@@ -83,9 +83,9 @@
     :param src_path: The path to search in.
     :return: An iterable of "enum" modules found in packages in src_path.
     """
-    return filter(None, [
-        get_module(src_path, package, 'enum')
-        for package in os.listdir(src_path)])
+    dirs = sorted(os.listdir(src_path))
+    modules = [get_module(src_path, package, 'enum') for package in dirs]
+    return filter(None, modules)
 
 
 def is_enum(item):