Minor cleanup of some recipe framework code.

scripts/slave/recipe_loader.py

Index: scripts/slave/recipe_loader.py
diff --git a/scripts/slave/recipe_loader.py b/scripts/slave/recipe_loader.py
index e4684168f1afbc0249909f12c22a50da332f569f..a324cfa9b6058e383b9c76c113fdef027c679224 100644
--- a/scripts/slave/recipe_loader.py
+++ b/scripts/slave/recipe_loader.py
@@ -10,18 +10,27 @@ from functools import wraps
 
 from .recipe_util import RECIPE_DIRS, MODULE_DIRS
 from .recipe_api import RecipeApi
+from .recipe_config import ConfigContext
 from .recipe_test_api import RecipeTestApi, DisabledTestData, ModuleTestData
 
 
 def load_recipe_modules(mod_dirs):
   def patchup_module(name, submod):
+    """Finds framework related classes and functions in a |submod| and adds

[Vadim Sh., 2014/03/05 06:09:30]

That was unclear for me from the first glance.

[iannucci, 2014/03/05 20:43:37]

Thanks. This docstring lg.

+    them to |submod| as top level constants with well known names such as
+    API, CONFIG_CTX and TEST_API.
+
+    |submod| is a recipe module (akin to python package) with submodules such as

[iannucci, 2014/03/05 20:43:37]

We should find a standard way to document types. Some type declarations in
docstrings are even supported by tools like YouCompleteMe (which uses jedi:
http://jedi.jedidjah.ch/en/latest/docs/features.html#type-hinting).

For that matter, we should really have a proper doc browser...

[Vadim Sh., 2014/03/05 21:40:38]

With current level of magicallity doc browser wouldn't be only partially useful.
For example, I don't think it documents inner functions, like that one.

Also I'm kind of on the fence regarding strict type documentation without a way
to enforce the types. I generally prefer to do asserts if specific subtype
matters.

[iannucci, 2014/03/05 21:58:30]

Maybe. I think it would be more useful than you might think. I'd especially like
a way to write markdown and then insert links/snapshots of
sourcecode/docstrings.
Fair enough. YCM can also read `assert isinstance(x, type)` :)

+    'api', 'config', 'test_api'. This function scans through dicts of that
+    submodules to find subclasses of RecipeApi, RecipeTestApi, etc.
+    """
     submod.NAME = name
     submod.CONFIG_CTX = getattr(submod, 'CONFIG_CTX', None)
     submod.DEPS = frozenset(getattr(submod, 'DEPS', ()))
 
     if hasattr(submod, 'config'):
       for v in submod.config.__dict__.itervalues():
-        if hasattr(v, 'I_AM_A_CONFIG_CTX'):

[Vadim Sh., 2014/03/05 06:09:30]

That was WTF moment...

[iannucci, 2014/03/05 20:43:37]

:D You're deep into the Old Code here ^_^.

Looking at the modern config_ctx, it's totally dumb not to have it be a class...
it used to be a lot simpler....

+        if isinstance(v, ConfigContext):
           assert not submod.CONFIG_CTX, (
             'More than one configuration context: %s' % (submod.config))
           submod.CONFIG_CTX = v
@@ -115,8 +124,8 @@ def load_recipe_modules(mod_dirs):
     imp.release_lock()
 
 
-def CreateApi(mod_dirs, names, test_data=DisabledTestData(), required=None,
-              optional=None, kwargs=None):
+def create_api(mod_dirs, names, test_data=DisabledTestData(), required=None,

[Vadim Sh., 2014/03/05 06:09:30]

I really can't stand mixed code styles in a single file... This stuff isn't
supposed to be used by recipes directly, and engine seems to be using mostly
lowercase_letters_with_underscores.

[Vadim Sh., 2014/03/05 06:09:30]

Also this function is a big WTF for me, I still have very vague idea of how it
works.

[iannucci, 2014/03/05 20:43:37]

yep. I think this slipped through a codereview

[iannucci, 2014/03/05 20:43:37]

Basically it creates a RecipeApi instance, with all module dependencies (and
test_api's) correctly injected (along with their dependencies, etc.). 

This would probably be more correctly modeled with a recursive function passing
a global dictionary of {mod_name: mod_instance} down through all the layers. I'm
not sure why I didn't do that.

On that note, we should start adding unit tests for this engine stuff (or at
least for the new engine stuff). machenbach did that for his rewrite of the
generator-unfolding code.

+               optional=None, kwargs=None):
   """
   Given a list of module names, return an instance of RecipeApi which contains
   those modules as direct members.
@@ -126,9 +135,11 @@ def CreateApi(mod_dirs, names, test_data=DisabledTestData(), required=None,
   module.
 
   Args:
-    names (list): A list of module names to include in the returned RecipeApi.
     mod_dirs (list): A list of paths to directories which contain modules.
+    names (list): A list of module names to include in the returned RecipeApi.
     test_data (TestData): ...
+    required: a pair such as ('API', RecipeApi).

[Vadim Sh., 2014/03/05 06:09:30]

I really tried to come up with a better doc string but couldn't :D Still "a
pair" bit is helpful, since from the first glance it's not obvious (I though it
was a list of required stuff).

+    optional: a pair such as ('TEST_API', RecipeTestApi).
     kwargs: Data passed to each module api. Usually this will contain:
         properties (dict): the properties dictionary (used by the properties
             module)
@@ -162,15 +173,18 @@ def CreateApi(mod_dirs, names, test_data=DisabledTestData(), required=None,
                                            test_data=mod_test, **kwargs)
       if optional:
         api = getattr(module, optional[0], None) or optional[1]
+        # TODO(vadimsh): Why not pass **kwargs here as well? There's
+        # an assumption here that optional[1] is always RecipeTestApi subclass
+        # (that doesn't need kwargs).

[Vadim Sh., 2014/03/05 06:09:30]

I dislike this asymmetry...

[iannucci, 2014/03/05 20:43:37]

Yeah me too :(. There shouldn't be kwargs at all. Again, carry over from days
past. Basically they're used so they show up in the module's __init__. I think
they're only /actually/ used to pass the properties dictionary, but originally
we thought there would be lots of other inputs to various modules.

[Vadim Sh., 2014/03/05 21:40:38]

It's used by properties and step_history modules. I was considering to use it to
pass 'engine' instance that implements new recipe engine. I'd like to discuss
this offline.

         inst_maps[optional[0]][name] = api(module=module,
                                            test_data=mod_test)
 
   map(create_maps, names)
 
   if required:
-    MapDependencies(dep_map, inst_maps[required[0]])
+    map_dependencies(dep_map, inst_maps[required[0]])
   if optional:
-    MapDependencies(dep_map, inst_maps[optional[0]])
+    map_dependencies(dep_map, inst_maps[optional[0]])
     if required:
       for name, module in inst_maps[required[0]].iteritems():
         module.test_api = inst_maps[optional[0]][name]
@@ -178,7 +192,7 @@ def CreateApi(mod_dirs, names, test_data=DisabledTestData(), required=None,
   return inst_maps[(required or optional)[0]][None]
 
 
-def MapDependencies(dep_map, inst_map):
+def map_dependencies(dep_map, inst_map):
   # NOTE: this is 'inefficient', but correct and compact.
   dep_map = copy.deepcopy(dep_map)
   while dep_map:
@@ -201,18 +215,18 @@ def MapDependencies(dep_map, inst_map):
     assert did_something, 'Did nothing on this loop. %s' % dep_map
 
 
-def CreateTestApi(names):
-  return CreateApi(MODULE_DIRS(), names, optional=('TEST_API', RecipeTestApi))
+def create_test_api(names):
+  return create_api(MODULE_DIRS(), names, optional=('TEST_API', RecipeTestApi))
 
 
-def CreateRecipeApi(names, test_data=DisabledTestData(), **kwargs):
-  return CreateApi(MODULE_DIRS(), names, test_data=test_data, kwargs=kwargs,
-                   required=('API', RecipeApi),
-                   optional=('TEST_API', RecipeTestApi))
+def create_recipe_api(names, test_data=DisabledTestData(), **kwargs):
+  return create_api(MODULE_DIRS(), names, test_data=test_data, kwargs=kwargs,
+                    required=('API', RecipeApi),
+                    optional=('TEST_API', RecipeTestApi))
 
 
-def Cached(f):
-  """Caches/memoizes a unary function.
+def cached(f):

[iannucci, 2014/03/05 20:43:37]

maybe this should move to recipe_util.py?

Probably should also be named 'cached_unary'

[Vadim Sh., 2014/03/05 21:40:38]

Done.

+  """Caches/memoizes an unary function.
 
     If the function throws an exception, the cache table will not be updated.
   """
@@ -239,8 +253,8 @@ class ModuleObject(object):
       setattr(self, k, v)
 
 
-@Cached
-def LoadRecipe(recipe):
+@cached
+def load_recipe(recipe):
   # If the recipe is specified as "module:recipe", then it is an recipe
   # contained in a recipe_module as an example. Look for it in the modules
   # imported by load_recipe_modules instead of the normal search paths.