[step] Remove config indirection.

recipe_engine/recipe_api.py

Index: recipe_engine/recipe_api.py
diff --git a/recipe_engine/recipe_api.py b/recipe_engine/recipe_api.py
index 6ec91c3137bcfc74aa533b73cbc9083ce8df6773..7d1d4666db41d81fd2d206dac5a229dd8e772a77 100644
--- a/recipe_engine/recipe_api.py
+++ b/recipe_engine/recipe_api.py
@@ -16,8 +16,7 @@ from functools import wraps
 
 from .recipe_test_api import DisabledTestData, ModuleTestData
 from .config import Single
-
-from .util import ModuleInjectionSite
+from .util import ModuleInjectionSite, Placeholder
 
 
 class UnknownRequirementError(object):
@@ -186,17 +185,28 @@ class StepClient(object):
           'result.')
     return self._engine._step_stack[-1].step_result
 
-  def run_step(self, step_dict):
+  def make_trigger_spec(self, **trigger_spec):
+    """Returns (TriggerSpec): A trigger spec for a StepConfig.
+
+    Args:
+      trigger_spec: Keyword arguments to use to instantiate a TriggerSpec.
+
+    Returns:
+      An instantiated, normalized TriggerSpec object.
+    """
+    return TriggerSpec.create(**trigger_spec)

[iannucci, 2017/06/12 18:20:08]

I don't like this method... the caller should just create TriggerSpec objects
directly.

[dnj, 2017/06/12 19:00:03]

Done.

+
+  def run_step(self, **step_config):
     """
-    Runs a step.
+    Runs a step from a StepConfig.
 
     Args:
-      step_dict (dict): A step dictionary to run.
+      step_config: Keyword arguments to use to instantiate a StepConfig.
 
     Returns:
       A StepData object containing the result of running the step.
     """
-    return self._engine.run_step(StepConfig.create(**step_dict))
+    return self._engine.run_step(StepConfig.create(**step_config))

[iannucci, 2017/06/12 18:20:08]

Same here, we should just assert that step_config isa StepConfig

[dnj, 2017/06/12 19:00:03]

Done.

 
 
 class DependencyManagerClient(object):
@@ -211,62 +221,47 @@ class DependencyManagerClient(object):
     return self._engine.depend_on(recipe, properties, **kwargs)
 
 
-_TriggerSpec = collections.namedtuple('_TriggerSpec',
-    ('bucket', 'builder_name', 'properties', 'buildbot_changes', 'tags',
-     'critical'))
+class StepConfig(collections.namedtuple('_StepConfig', (
+    'name', 'base_name', 'cmd', 'cwd', 'env', 'allow_subannotations',
+    'trigger_specs', 'timeout', 'infra_step', 'stdout', 'stderr', 'stdin',
+    'ok_ret', 'step_test_data', 'nest_level'))):
 
-class TriggerSpec(_TriggerSpec):
-  """
-  TriggerSpec is the internal representation of a raw trigger step. You should
-  use the standard 'step' recipe module, which will construct trigger specs
-  via API.
-  """
-
-  @classmethod
-  def _create(cls, builder_name, bucket=None, properties=None,
-              buildbot_changes=None, tags=None, critical=None):
-    """Creates a new TriggerSpec instance from its step API dictionary
-    keys/values.
-
-    Args:
-      builder_name (str): The name of the builder to trigger.
-      bucket (str or None): The name of the trigger bucket.
-      properties (dict or None): Key/value properties dictionary.
-      buildbot_changes (list or None): Optional list of BuildBot change dicts.
-      tags (list or None): Optional list of tag strings.
-      critical (bool or None): If true and triggering fails asynchronously, fail
-          the entire build. If None, the step defaults to being True.
-    """
-    if not isinstance(buildbot_changes, (types.NoneType, list)):
-      raise ValueError('buildbot_changes must be a list')
-
-    return cls(
-        bucket=bucket,
-        builder_name=builder_name,
-        properties=properties,
-        buildbot_changes=buildbot_changes,
-        tags=tags,
-        critical=bool(critical) if critical is not None else (True),
-    )
-
-  def _render_to_dict(self):
-    d = dict((k, v) for k, v in self._asdict().iteritems() if v)
-    if d['critical']:
-      d.pop('critical')
-    return d
-
-
-_StepConfig = collections.namedtuple('_StepConfig',
-    ('name', 'base_name', 'cmd', 'cwd', 'env', 'allow_subannotations',
-     'trigger_specs', 'timeout', 'infra_step', 'stdout', 'stderr', 'stdin',
-     'ok_ret', 'step_test_data', 'nest_level'))
-
-class StepConfig(_StepConfig):
   """
   StepConfig is the representation of a raw step as the recipe_engine sees it.
   You should use the standard 'step' recipe module, which will construct and
   pass this data to the engine for you, instead. The only reason why you would
-  need to worry about this object is if you're modifying the step module itself.
+  need to worry about this object is if you're modifying the step module
+  itself.
+
+  Fields:
+    name (str): name of the step, will appear in buildbots waterfall
+    base_name (str): the base name of the step. If the step has a derived
+        name (e.g., nested may be concatenated with its parent), this is the
+        name component of just this step. If None, this will be set to "name".
+    cmd: command to run. Acceptable types: str, Path, Placeholder, or None.
+    cwd (str or None): absolute path to working directory for the command
+    env (dict): overrides for environment variables, described above.
+    allow_subannotations (bool): if True, lets the step emit its own
+        annotations. NOTE: Enabling this can cause some buggy behavior. Please
+        strongly consider using step_result.presentation instead. If you have
+        questions, please contact infra-dev@chromium.org.
+    trigger_specs: a list of trigger specifications, see also _trigger_builds.
+    timeout: if not None, a datetime.timedelta for the step timeout.
+    infra_step: if True, this is an infrastructure step. Failures will raise
+        InfraFailure instead of StepFailure.
+    stdout: Placeholder to put step stdout into. If used, stdout won't appear
+        in annotator's stdout (and |allow_subannotations| is ignored).
+    stderr: Placeholder to put step stderr into. If used, stderr won't appear
+        in annotator's stderr.
+    stdin: Placeholder to read step stdin from.
+    ok_ret (iter): set of return codes allowed. If the step process returns
+        something not on this list, it will raise a StepFailure (or
+        InfraFailure if infra_step is True). If omitted, {0} will be used.
+    step_test_data (func -> recipe_test_api.StepTestData): A factory which
+        returns a StepTestData object that will be used as the default test
+        data for this step. The recipe author can override/augment this object
+        in the GenTests function.
+    nest_level (int): the step's nesting level.
 
   The optional "env" parameter provides optional overrides for environment
   variables. Each value is % formatted with the entire existing os.environ. A
@@ -291,70 +286,66 @@ class StepConfig(_StepConfig):
   ))
 
   @classmethod
-  def create(cls, name, base_name=None, cmd=None, cwd=None, env=None,
-             allow_subannotations=None, trigger_specs=None, timeout=None,
-             infra_step=None, stdout=None, stderr=None, stdin=None,
-             ok_ret=None, step_test_data=None, step_nest_level=None):
-    """
-    Initializes a new StepConfig step API dictionary.
-
-    Args:
-      name (str): name of the step, will appear in buildbots waterfall
-      base_name (str): the base name of the step. If the step has a derived
-          name (e.g., nested may be concatenated with its parent), this is the
-          name component of just this step. If None, this will be set to "name".
-      cmd: command to run. Acceptable types: str, Path, Placeholder, or None.
-      cwd (str or None): absolute path to working directory for the command
-      env (dict): overrides for environment variables, described above.
-      allow_subannotations (bool): if True, lets the step emit its own
-          annotations. NOTE: Enabling this can cause some buggy behavior. Please
-          strongly consider using step_result.presentation instead. If you have
-          questions, please contact infra-dev@chromium.org.
-      trigger_specs: a list of trigger specifications, see also _trigger_builds.
-      timeout: if not None, a datetime.timedelta for the step timeout.
-      infra_step: if True, this is an infrastructure step. Failures will raise
-          InfraFailure instead of StepFailure.
-      stdout: Placeholder to put step stdout into. If used, stdout won't appear
-          in annotator's stdout (and |allow_subannotations| is ignored).
-      stderr: Placeholder to put step stderr into. If used, stderr won't appear
-          in annotator's stderr.
-      stdin: Placeholder to read step stdin from.
-      ok_ret (iter): set of return codes allowed. If the step process returns
-          something not on this list, it will raise a StepFailure (or
-          InfraFailure if infra_step is True). If omitted, {0} will be used.
-      step_test_data (func -> recipe_test_api.StepTestData): A factory which
-          returns a StepTestData object that will be used as the default test
-          data for this step. The recipe author can override/augment this object
-          in the GenTests function.
-      step_nest_level (int): the step's nesting level.
-    """
-    return cls(
-        name=name,
-        base_name=(base_name or name),
-        cmd=cmd,
-        cwd=cwd,
-        env=env,
-        allow_subannotations=bool(allow_subannotations),
-        trigger_specs=[TriggerSpec._create(**trig)
-                       for trig in (trigger_specs or ())],
-        timeout=timeout,
-        infra_step=bool(infra_step),
-        stdout=stdout,
-        stderr=stderr,
-        stdin=stdin,
-        ok_ret=frozenset(ok_ret or (0,)),
-        step_test_data=step_test_data,
-        nest_level=int(step_nest_level or 0),
+  def create(cls, **kwargs):
+    for field in cls._fields:
+      kwargs.setdefault(field, None)
+    sc = cls(**kwargs)
+
+    return sc._replace(
+        cmd=[(x if isinstance(x, Placeholder) else str(x))
+             for x in (sc.cmd or ())],
+        cwd=(str(sc.cwd) if sc.cwd else (None)),
+        base_name=sc.base_name or sc.name,
+        allow_subannotations=bool(sc.allow_subannotations),
+        trigger_specs=sc.trigger_specs or (),
+        infra_step=bool(sc.infra_step),
+        ok_ret=frozenset(sc.ok_ret or (0,)),
+        nest_level=int(sc.nest_level or 0),
     )
 
   def render_to_dict(self):
-    self = self._replace(
+    sc = self._replace(
         trigger_specs=[trig._render_to_dict()
                        for trig in (self.trigger_specs or ())],
     )
-    return dict((k, v) for k, v in self._asdict().iteritems()
-                if (v or k in self._RENDER_WHITELIST)
-                and k not in self._RENDER_BLACKLIST)
+    return dict((k, v) for k, v in sc._asdict().iteritems()
+                if (v or k in sc._RENDER_WHITELIST)
+                and k not in sc._RENDER_BLACKLIST)
+
+
+class TriggerSpec(collections.namedtuple('_TriggerSpec', (
+    'bucket', 'builder_name', 'properties', 'buildbot_changes', 'tags',
+    'critical'))):
+
+  """
+  TriggerSpec is the internal representation of a raw trigger step. You should
+  use the standard 'step' recipe module, which will construct trigger specs
+  via API.
+
+  Fields:
+    builder_name (str): The name of the builder to trigger.
+    bucket (str or None): The name of the trigger bucket.
+    properties (dict or None): Key/value properties dictionary.
+    buildbot_changes (list or None): Optional list of BuildBot change dicts.
+    tags (list or None): Optional list of tag strings.
+    critical (bool or None): If true and triggering fails asynchronously, fail
+        the entire build. If None, the step defaults to being True.
+  """
+
+  @classmethod
+  def create(cls, **kwargs):
+    for field in cls._fields:
+      kwargs.setdefault(field, None)
+    trig = cls(**kwargs)
+    return trig._replace(
+        critical=bool(trig.critical),
+    )
+
+  def _render_to_dict(self):
+    d = dict((k, v) for k, v in self._asdict().iteritems() if v)
+    if d['critical']:
+      d.pop('critical')
+    return d
 
 
 class StepFailure(Exception):