Add better documentation, trigger namedtuple.

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 fef517232a4354792d4b93efbf0850b62dee1280..7b6a862f351027a9824788dd7f746d261c9d179c 100644
--- a/recipe_engine/recipe_api.py
+++ b/recipe_engine/recipe_api.py
@@ -19,57 +19,153 @@ from .util import ModuleInjectionSite
 from . import field_composer
 
 
-_StepConfig = collections.namedtuple('StepConfig',
+_TriggerSpec = collections.namedtuple('_TriggerSpec',
+    ('bucket', 'builder_name', 'properties', 'buildbot_changes', 'tags',
+     'critical'))
+
+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 _make_from_api_dict(cls, trig):

[iannucci, 2016/09/12 22:35:16]

could we **explode this and add docstring?

+    builder_name = trig.pop('builder_name')
+    if not builder_name:
+      raise ValueError('Trigger spec: builder_name is not set')
+
+    changes = trig.pop('buildbot_changes', None)
+    if changes is not None:
+      assert isinstance(changes, list), 'buildbot_changes must be a list'

[iannucci, 2016/09/12 22:35:16]

raise valueerror

[dnj, 2016/09/12 23:30:11]

Done.

+
+    inst = cls(
+        bucket=trig.pop('bucket', None),
+        builder_name=builder_name,
+        properties=trig.pop('properties', None),
+        buildbot_changes=changes,
+        tags=trig.pop('tags', None),
+        critical=trig.pop('critical', True),
+    )
+
+    # Confirm that all dictionary keys have been consumed.
+    if trig:
+      unknown_keys = sorted(trig.iterkeys())
+      raise KeyError('Unknown trigger spec keys: %s' % (

[iannucci, 2016/09/12 22:35:16]

I think this is still ValueError. I think KeyError is meant for "key is not in
the dictionary" type things.

[dnj, 2016/09/12 23:30:11]

Done.

+          ', '.join(unknown_keys)))
+
+    return inst
+
+  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', 'cmd', 'cwd', 'env', 'allow_subannotations', 'trigger_specs',
-      'timeout', 'infra_step', 'stdout', 'stderr', 'stdin', 'ok_ret',
-      'step_test_data', 'nest_level'))
+     '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.
+
+  The optional "env" parameter provides optional overrides for environment
+  variables. Each value is % formatted with the entire existing os.environ. A
+  value of `None` will remove that envvar from the environ. e.g.
+
+    {
+        "envvar": "%(envvar)s;%(envvar2)s;extra",
+        "delete_this": None,
+        "static_value": "something",
+    }
+
   StepConfig parameters:
-    name: name of the step, will appear in buildbots waterfall
-    cmd: command to run, list of one or more strings
-    cwd: absolute path to working directory for the command
-    env: dict with overrides for environment variables
-    allow_subannotations: if True, lets the step emit its own annotations
+    name (str): name of the step, will appear in buildbots waterfall
+    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.
-    stdout: Path to a file to put step stdout into. If used, stdout won't
-        appear in annotator's stdout (and |allow_subannotations| is
-        ignored).
-    stderr: Path to a file to put step stderr into. If used, stderr won't
-        appear in annotator's stderr.
-    stdin: Path to a file to read step stdin from.
-    ok_ret: Allowed return code list.
-    step_test_data: Possible associated step test data.
-    nest_level: the step's nesting level.
+    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

[iannucci, 2016/09/12 22:35:16]

if you want to say this is an iter, then we should also convert it to a
frozenset down below (since iter doesn't guarantee that it can be iterated over
repeatedly. A generator is an iter, for example).

[dnj, 2016/09/12 23:30:11]

Was converting to a set, but frozenset is even better.

+        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.
   """
 
+  _RENDER_WHITELIST=frozenset((
+    'cmd',
+  ))
+
+  _RENDER_BLACKLIST=frozenset((
+    'nest_level',
+    'ok_ret',
+    'infra_step',
+    'step_test_data',
+  ))
+
+  @classmethod
+  def _make_from_api_dict(cls, step):
+    step_config = StepConfig(
+        name=step.pop('name'),
+        cmd=step.pop('cmd', None),
+        cwd=step.pop('cwd', None),
+        env=step.pop('env', None),
+        allow_subannotations=step.pop('allow_subannotations', False),
+        trigger_specs=[TriggerSpec._make_from_api_dict(trig)
+                       for trig in step.pop('trigger_specs', ())],
+        timeout=step.pop('timeout', None),
+        infra_step=step.pop('infra_step', False),
+        stdout=step.pop('stdout', None),
+        stderr=step.pop('stderr', None),
+        stdin=step.pop('stdin', None),
+        ok_ret=set(step.pop('ok_ret', {0})),
+        step_test_data=step.pop('step_test_data', None),
+        nest_level=int(step.pop('step_nest_level', 0)),
+    )
+
+    # Confirm that all dictionary keys have been consumed.
+    if step:
+      unknown_keys = sorted(step.iterkeys())
+      raise KeyError('Unknown step dictionary keys: %s' % (
+          ', '.join(unknown_keys)))
+
+    return step_config
+
+  def render_to_dict(self):
+    self = 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)
+
 
 def _make_step_config(**step):
-  """Galvanize a step dictionary into a formal StepConfig."""
-  step_config = StepConfig(
-      name=step.pop('name'),
-      cmd=step.pop('cmd', None),
-      cwd=step.pop('cwd', None),
-      env=step.pop('env', None),
-      allow_subannotations=step.pop('allow_subannotations', False),
-      trigger_specs=step.pop('trigger_specs', ()),
-      timeout=step.pop('timeout', None),
-      infra_step=step.pop('infra_step', False),
-      stdout=step.pop('stdout', None),
-      stderr=step.pop('stderr', None),
-      stdin=step.pop('stdin', None),
-      ok_ret=step.pop('ok_ret', {0}),
-      step_test_data=step.pop('step_test_data', None),
-      nest_level=step.pop('step_nest_level', 0),
-  )
-  if step:
-    unknown_keys = sorted(step.iterkeys())
-    raise KeyError('Unknown step dictionary keys: %s' % (
-        ', '.join(unknown_keys)))
-  return step_config
+  """Convert a step API dictionary into a formal StepConfig."""
+  return StepConfig._make_from_api_dict(step)
 
 
 class StepFailure(Exception):