launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Gavin Panella has proposed merging lp:~allenap/maas/convert-enums-redux into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~allenap/maas/convert-enums-redux/+merge/104568

Depending on "enums" - a phony target - in Makefile was making some things get rebuilt again and again. Instead I changed it to depend on the target file instead, which make is much happier resolving... and then I got my yak attack on.

I also changed convert-enums.py to accept a list of file names, instead of repeating the work that make had just done to find the modules. I also moved it into the src/ tree and added tests for the more interesting functions.

-- 
https://code.launchpad.net/~allenap/maas/convert-enums-redux/+merge/104568
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/convert-enums-redux into lp:maas.

=== modified file 'Makefile'
--- Makefile	2012-05-02 03:58:51 +0000
+++ Makefile	2012-05-03 14:56:26 +0000
@@ -1,4 +1,9 @@
-PYTHON = python2.7
+python := python2.7
+
+# Python enum modules.
+py_enums := $(wildcard src/*/enum.py)
+# JavaScript enum module (not modules).
+js_enums := src/maasserver/static/js/enums.js
 
 build: \
     bin/buildout \
@@ -6,19 +11,19 @@
     bin/twistd.pserv bin/test.pserv \
     bin/twistd.txlongpoll \
     bin/py bin/ipy \
-    enums
+    $(js_enums)
 
 all: build doc
 
 bin/buildout: bootstrap.py distribute_setup.py
-	$(PYTHON) bootstrap.py --distribute --setup-source distribute_setup.py
+	$(python) bootstrap.py --distribute --setup-source distribute_setup.py
 	@touch --no-create $@  # Ensure it's newer than its dependencies.
 
-bin/maas: bin/buildout buildout.cfg versions.cfg setup.py enums
+bin/maas: bin/buildout buildout.cfg versions.cfg setup.py $(js_enums)
 	bin/buildout install maas
 	@touch --no-create $@
 
-bin/test.maas: bin/buildout buildout.cfg versions.cfg setup.py enums
+bin/test.maas: bin/buildout buildout.cfg versions.cfg setup.py $(js_enums)
 	bin/buildout install maas-test
 	@touch --no-create $@
 
@@ -49,7 +54,7 @@
 dev-db:
 	utilities/maasdb start ./db/ disposable
 
-test: bin/test.maas bin/test.pserv enums
+test: bin/test.maas bin/test.pserv $(js_enums)
 	bin/test.maas
 	bin/test.pserv
 
@@ -80,20 +85,16 @@
 doc: bin/sphinx docs/api.rst
 	bin/sphinx
 
-# JavaScript enums module, generated from python enums modules.
-JSENUMS = src/maasserver/static/js/enums.js
-
-# Generate JavaScript enums from python enums.
-enums: $(JSENUMS)
-
-$(JSENUMS): utilities/convert-enums.py src/*/enum.py
-	utilities/convert-enums.py --src=src >$@
+enums: $(js_enums)
+
+$(js_enums): bin/py src/maas/utils/jsenums.py $(py_enums)
+	 bin/py -m src/maas/utils/jsenums $(py_enums) > $@
 
 clean:
 	find . -type f -name '*.py[co]' -print0 | xargs -r0 $(RM)
 	find . -type f -name '*~' -print0 | xargs -r0 $(RM)
 	$(RM) -r media/demo/* media/development
-	$(RM) $(JSENUMS)
+	$(RM) $(js_enums)
 
 distclean: clean stop
 	utilities/maasdb delete-cluster ./db/

=== added directory 'src/maas/utils'
=== added file 'src/maas/utils/__init__.py'
=== renamed file 'utilities/convert-enums.py' => 'src/maas/utils/jsenums.py'
--- utilities/convert-enums.py	2012-05-02 17:13:01 +0000
+++ src/maas/utils/jsenums.py	2012-05-03 14:56:26 +0000
@@ -8,8 +8,9 @@
 Running this script produces a source text containing the JavaScript
 equivalents of the same enums, so that JavaScript code can make use of them.
 
-The script takes one option: --src=DIRECTORY.  DIRECTORY is where the MAAS
-modules (maasserver, metadataserver, provisioningserver) can be found.
+The script takes the filename of the enum modules. Each will be compiled and
+executed in an empty namespace, though they will have access to other MAAS
+libraries, though not necessarily their dependencies.
 
 The resulting JavaScript module is printed to standard output.
 """
@@ -21,14 +22,18 @@
     )
 
 __metaclass__ = type
+__all__ = []
 
 from argparse import ArgumentParser
 from datetime import datetime
+from itertools import chain
 import json
-import os.path
+from operator import attrgetter
 import sys
 from textwrap import dedent
 
+from maasserver.utils import map_enum
+
 # Header.  Will be written on top of the output.
 header = dedent("""\
     /*
@@ -44,43 +49,10 @@
     """
     % {'script': sys.argv[0], 'timestamp': datetime.now()})
 
-
 # Footer.  Will be written at the bottom.
 footer = "}, '0.1');"
 
 
-def get_module(src_path, package):
-    """Attempt to load a given module.
-
-    This makes some assumptions about directory structure: it is assumed
-    that the module is directly in a package of the given name, which in turn
-    should be directly in the search path.
-
-    :param src_path: The path to search in.
-    :param package: The package to load the requested module from.
-    :param name: Name of module to load.
-    :return: The imported module, or None if it was not found.
-    """
-    if os.path.isfile(os.path.join(src_path, package, "enum.py")):
-        return __import__('.'.join([package, 'enum']), level=0, fromlist=True)
-    else:
-        return None
-
-
-def find_enum_modules(src_path):
-    """Find MAAS "enum" modules in the packages in src_path.
-
-    This assumes that all MAAS enums can be found in
-    <src_path>/<package>/enum.py.
-
-    :param src_path: The path to search in.
-    :return: An iterable of "enum" modules found in packages in src_path.
-    """
-    dirs = sorted(os.listdir(src_path))
-    modules = [get_module(src_path, package) for package in dirs]
-    return filter(None, modules)
-
-
 def is_enum(item):
     """Does the given python item look like an enum?
 
@@ -90,16 +62,22 @@
     return isinstance(item, type) and item.__name__.isupper()
 
 
-def get_enum_classes(module):
-    """Collect all enum classes exported from loaded `module`."""
-    return filter(is_enum, [module.__dict__[name] for name in module.__all__])
+def get_enum_classes(namespace):
+    """Collect all enum classes exported from `namespace`."""
+    return filter(is_enum, namespace.values())
+
+
+def get_enums(filename):
+    namespace = {}
+    with open(filename, "rbU") as fd:
+        source = fd.read()
+    code = compile(source, filename, "exec")
+    exec(code, namespace)
+    return get_enum_classes(namespace)
 
 
 def serialize_enum(enum):
     """Represent a MAAS enum class in JavaScript."""
-    # Import lazily to make use of initialized path.
-    from maasserver.utils import map_enum
-
     return "module.%s = %s;\n" % (
         enum.__name__,
         json.dumps(map_enum(enum), indent=4, sort_keys=True))
@@ -107,24 +85,22 @@
 
 def parse_args():
     """Parse options & arguments."""
-    default_src = os.path.join(os.path.dirname(sys.path[0]), 'src')
     parser = ArgumentParser(
         "Generate JavaScript enums based on python enums modules")
     parser.add_argument(
-        '--src', metavar='SOURCE_DIRECTORY', type=str, default=default_src,
-        help="Look for the MAAS packages in SOURCE_DIRECTORY.")
+        'sources', metavar="FILENAME", nargs='+',
+        help="File to search for enums.")
     return parser.parse_args()
 
 
-def main(args):
-    enum_modules = find_enum_modules(args.src)
-    enums = sum((get_enum_classes(module) for module in enum_modules), [])
+def dump(source_filenames):
+    enums = chain.from_iterable(
+        get_enums(filename) for filename in source_filenames)
+    enums = sorted(enums, key=attrgetter("__name__"))
     dumps = [serialize_enum(enum) for enum in enums]
-    print("\n".join([header] + dumps + [footer]))
+    return "\n".join([header] + dumps + [footer])
 
 
 if __name__ == "__main__":
     args = parse_args()
-    # Add src directory so that we can import from MAAS packages.
-    sys.path.append(args.src)
-    main(args)
+    print(dump(args.sources))

=== added directory 'src/maas/utils/tests'
=== added file 'src/maas/utils/tests/__init__.py'
=== added file 'src/maas/utils/tests/test_jsenums.py'
--- src/maas/utils/tests/test_jsenums.py	1970-01-01 00:00:00 +0000
+++ src/maas/utils/tests/test_jsenums.py	2012-05-03 14:56:26 +0000
@@ -0,0 +1,59 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for `maas.utils.jsenums`."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = []
+
+from maas.utils.jsenums import (
+    dump,
+    footer,
+    get_enums,
+    header,
+    serialize_enum,
+    )
+from maasserver.utils import map_enum
+from maastesting.testcase import TestCase
+
+
+class ENUM:
+    ALICE = 1
+    BOB = 2
+
+
+class Test(TestCase):
+
+    def test_serialize_enum(self):
+        # The name is used correctly, the keys are sorted, and everything is
+        # indented correctly.
+        self.assertEqual(
+            u'module.ENUM = {\n'
+            u'    "ALICE": 1, \n'
+            u'    "BOB": 2\n'
+            u'};\n',
+            serialize_enum(ENUM))
+
+    def test_get_enums(self):
+        # This file contains a single enum, named "ENUM".
+        enums = get_enums(__file__)
+        self.assertEqual(["ENUM"], [enum.__name__ for enum in enums])
+        [enum] = enums
+        # Because the module has been executed in a different namespace, the
+        # enum we've found is not the same object as the one in the current
+        # global namespace.
+        self.assertIsNot(ENUM, enum)
+        # It does, however, have the same values.
+        self.assertEqual(map_enum(ENUM), map_enum(enum))
+
+    def test_dump(self):
+        self.assertEqual(header + "\n" + footer, dump([]))
+        self.assertEqual(
+            header + "\n" + serialize_enum(ENUM) + "\n" + footer,
+            dump([__file__]))