Minor cleanup of some recipe framework code.

scripts/slave/recipe_loader.py

 # Copyright 2013 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 import copy
 import imp
 import inspect
 import os
 import sys
 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
       assert submod.CONFIG_CTX, 'Config file, but no config context?'
 
     submod.API = getattr(submod, 'API', None)
     for v in submod.api.__dict__.itervalues():
       if inspect.isclass(v) and issubclass(v, RecipeApi):
         assert not submod.API, (
           'More than one Api subclass: %s' % submod.api)
@@ skipping 73 matching lines @@
 
       # Then import all the config extenders.
       for root in mod_dirs:
         if os.path.isdir(root):
           recursive_import(root)
     return sys.modules[RM]
   finally:
     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.
 
   So, if you pass ['foobar'], you'll get an instance back which contains a
   'foobar' attribute which itself is a RecipeApi instance from the 'foobar'
   module.
 
   Args:
+    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.
-    mod_dirs (list): A list of paths to directories which contain modules.
     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)
         step_history (OrderedDict): the step history object (used by the
             step_history module!)
   """
   kwargs = kwargs or {}
   recipe_modules = load_recipe_modules(mod_dirs)
 
   inst_maps = {}
@@ skipping 13 matching lines @@
       mod_test = DisabledTestData()
       if test_data.enabled:
         mod_test = test_data.mod_data.get(name, ModuleTestData())
 
       if required:
         api = getattr(module, required[0])
         inst_maps[required[0]][name] = api(module=module,
                                            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]
 
   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:
     did_something = False
     to_pop = []
     for api_name, deps in dep_map.iteritems():
       to_remove = []
       for dep in [d for d in deps if d not in dep_map]:
         # Grab the injection site
         obj = inst_map[api_name].m
         assert not hasattr(obj, dep)
         setattr(obj, dep, inst_map[dep])
         to_remove.append(dep)
         did_something = True
       map(deps.remove, to_remove)
       if not deps:
         to_pop.append(api_name)
         did_something = True
     map(dep_map.pop, to_pop)
     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.
   """
   cache = {}
   empty = object()
 
   @wraps(f)
   def cached_f(inp):
     cache_entry = cache.get(inp, empty)
     if cache_entry is empty:
       cache_entry = f(inp)
       cache[inp] = cache_entry
     return cache_entry
   return cached_f
 
 
 class NoSuchRecipe(Exception):
   pass
 
 
 class ModuleObject(object):
   def __init__(self, d):
     for k, v in d.iteritems():
       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.
   if ':' in recipe:
     module_name, example = recipe.split(':')
     assert example.endswith('example')
     RECIPE_MODULES = load_recipe_modules(MODULE_DIRS())
     try:
       return getattr(getattr(RECIPE_MODULES, module_name), example)
     except AttributeError:
@@ skipping 21 matching lines @@
 def loop_over_recipes():
   for path in RECIPE_DIRS():
     for recipe in find_recipes(
         path, lambda f: f.endswith('.py') and f[0] != '_'):
       yield recipe, recipe[len(path)+1:-len('.py')]
   for path in MODULE_DIRS():
     for recipe in find_recipes(
         path, lambda f: f.endswith('example.py')):
       module_name = os.path.dirname(recipe)[len(path)+1:]
       yield recipe, '%s:example' % module_name