Cleanup: Some simplifications in json_schema_compiler.

tools/json_schema_compiler/util_cc_helper.py

 # Copyright (c) 2012 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.
 
 _API_UTIL_NAMESPACE = 'json_schema_compiler::util'
 
 
 class UtilCCHelper(object):
   """A util class that generates code that uses
   tools/json_schema_compiler/util.cc.
@@ skipping 18 matching lines @@
     sub['type'] = self._type_manager.GetCppType(prop),
     if array_prop.optional:
       val = ('%(namespace)s::PopulateOptionalArrayFromDictionary'
           '(*%(src)s, "%(name)s", &%(dst)s)')
     else:
       val = ('%(namespace)s::PopulateArrayFromDictionary'
           '(*%(src)s, "%(name)s", &%(dst)s)')
 
     return val % sub
 
-  def PopulateArrayFromList(self, src, dst, optional):
+  def PopulateArrayFromList(self, src, dst, optional, items_are_binary):

[not at google - send to devlin, 2015/01/13 18:15:14]

For boolean flags I prefer to make it explicit, like binary=False then forcing
callers to write binary=<expression>

[pneubeck (no reviews), 2015/01/14 10:54:15]

Not sure how you could /force/ the caller to use named arguments, as he can
always just pass in positional arguments and python will be happy, AFAIK.

Done.

     """Generates code to get an array from src into dst.
 
     src: ListValue*
     dst: std::vector or scoped_ptr<std::vector>
     """
+
+    binary_tag = 'ITEMS_ARE_NOT_BINARY';
+    if items_are_binary:
+      binary_tag = 'ITEMS_ARE_BINARY';
     if optional:
-      val = '%(namespace)s::PopulateOptionalArrayFromList(*%(src)s, &%(dst)s)'
+      val = ('%(namespace)s::PopulateOptionalArrayFromList'
+          '(*%(src)s, %(namespace)s::%(binary_tag)s(), &%(dst)s)')
     else:
-      val = '%(namespace)s::PopulateArrayFromList(*%(src)s, &%(dst)s)'
+      val = ('%(namespace)s::PopulateArrayFromList'
+          '(*%(src)s, %(namespace)s::%(binary_tag)s(), &%(dst)s)')

[not at google - send to devlin, 2015/01/13 18:15:14]

Given there's even more duplication, you should be able to sugarise it a little
more, like

optional_or_required = 'Optional' if optional else ''
binary_tag = 'ITEMS_ARE_BINARY' if binary else 'ITEMS_ARE_NOT_BINARY'
return '%s::Populate%sFromList(*%s), %s::%s(), &%s)' % ...

(I don't particularly like named string substitution params in these cases, just
obfuscates the code IMO)

[pneubeck (no reviews), 2015/01/14 10:54:15]

Slight variation of what you proposed because concatenating identifiers is IMO a
no-go as it destroys grep-ability.

     return val % {
+      'binary_tag': binary_tag,
       'namespace': _API_UTIL_NAMESPACE,
       'src': src,
       'dst': dst
     }
 
   def CreateValueFromArray(self, src, optional):
     """Generates code to create a scoped_pt<Value> from the array at src.
 
     |src| The variable to convert, either a vector or scoped_ptr<vector>.
     |optional| Whether |type_| was optional. Optional types are pointers so
         must be treated differently.
     """
     if optional:
       name = 'CreateValueFromOptionalArray'
     else:
       name = 'CreateValueFromArray'
     return '%s::%s(%s)' % (_API_UTIL_NAMESPACE, name, src)
 
   def GetIncludePath(self):
     return '#include "tools/json_schema_compiler/util.h"'
 
   def GetValueTypeString(self, value, is_ptr=False):
     call = '.GetType()'
     if is_ptr:
       call = '->GetType()'
     return 'json_schema_compiler::util::ValueTypeToString(%s%s)' % (value, call)