Allow PRESUBMIT.py files to have old and new-style trybot specifications (as long as each file is c…

presubmit_support.py

Index: presubmit_support.py
diff --git a/presubmit_support.py b/presubmit_support.py
index 345c78eb6b8ec7caa676e7d6c0d624e737eafa70..0f0242874f936f102dfe5e7b51c2f9300ef4b9d1 100755
--- a/presubmit_support.py
+++ b/presubmit_support.py
@@ -1081,31 +1081,41 @@ def DoGetTrySlaves(change,
     if verbose:
       output_stream.write("Running default presubmit script.\n")
     fake_path = os.path.join(repository_root, 'PRESUBMIT.py')
-    results += executer.ExecPresubmitScript(
+    result = executer.ExecPresubmitScript(
         default_presubmit, fake_path, project, change)
+    # Ensure it's either all old-style or new-style.
+    if (all(isinstance(i, tuple) for i in result) or
+        all(isinstance(i, basestring) for i in result)):
+      results.extend(result)
+    else:
+      raise ValueError('PRESUBMIT.py returned invalid trybot specification!')
   for filename in presubmit_files:
     filename = os.path.abspath(filename)
     if verbose:
       output_stream.write("Running %s\n" % filename)
     # Accept CRLF presubmit script.
     presubmit_script = gclient_utils.FileRead(filename, 'rU')
-    results += executer.ExecPresubmitScript(
+    result = executer.ExecPresubmitScript(
         presubmit_script, filename, project, change)
+    if (all(isinstance(i, tuple) for i in result) or
+        all(isinstance(i, basestring) for i in result)):
+      results.extend(result)
+    else:
+      raise ValueError('%s returned invalid trybot specification!' % filename)
 

[Isaac (away), 2013/12/04 03:47:43]

if you make results a list of lists instead of flattening using extend, you
could mostly preserve the code below and keep the checks there.

-  if all(isinstance(i, tuple) for i in results):
-    # New-style [('bot', set(['tests']))] format.
-    slave_dict = {}
-    for result in results:
-      slave_dict.setdefault(result[0], set()).update(result[1])
-    slaves = list(slave_dict.iteritems())
-  elif all(isinstance(i, basestring) for i in results):
-    # Old-style ['bot'] format.
-    slaves = list(set(results))
-  else:
-    raise ValueError('PRESUBMIT.py returned invalid trybot specification!')
+
+  slave_dict = {}
+  old_style = filter(lambda x: isinstance(x, basestring), results)
+  new_style = filter(lambda x: isinstance(x, tuple), results)
+
+  for result in new_style:
+    slave_dict.setdefault(result[0], set()).update(result[1])

[Isaac (away), 2013/12/04 03:47:43]

now that we're on 2.7, how about collections.defaultdict?  Also generators are
usually preferred over filter:

slave_dict = collections.defaultdict(set)
for r in results:
  if isinstance(x, tuple):  # preferred style
    slave_dict[results[0]].update[results[1]]

# list of slave -> set of tests
slaves = slave_dict.items() 
slaves.extend(set(r for r in results if isinstance(r, basestring)))


Are you sure this is what you want?  This won't condense duplication of bots
between old and new styles.

[ghost stip (do not use), 2013/12/18 00:43:08]

we're not yet on 2.7, see crbug.com/328453.

filter is extremely concise here. generators are for nerds.
That's outside the scope of this CL.

+  slaves = list(slave_dict.iteritems())

[M-A Ruel, 2013/12/04 01:02:31]

slaves = slave_dict.items()

[ghost stip (do not use), 2013/12/18 00:43:08]

not sure if that's what we want, we want (bot, tests) tuple given by iteritems()

[M-A Ruel, 2013/12/18 00:52:43]

http://docs.python.org/2/library/stdtypes.html#dict.items

+
+  slaves += list(set(old_style))

[M-A Ruel, 2013/12/04 01:02:31]

slaves.extend(set(old_style))

 
   if slaves and verbose:
-    output_stream.write(', '.join(slaves))
+    output_stream.write(', '.join((str(x) for x in slaves)))
     output_stream.write('\n')
   return slaves