On July 11, 2009, Gary Poster wrote:
> Gary Poster has proposed merging
> lp:~gary/z3c.recipe.filetemplate/support-system-python into
> lp:~gary/z3c.recipe.filetemplate/trunk.
>
> Requested reviews:
>     Francis J. Lacoste (flacoste)
>
> Support the new zc.buildout 1.4.0+ include-site-packages option, and more.
>

Hi Gary,

This all looks good. My only concern is the new write_and_wait which I don't 
understand why it's necessary.

> === modified file 'z3c/recipe/filetemplate/__init__.py'

> @@ -195,17 +202,19 @@
>                  'Destinations already exist: %s. Please make sure that '
>                  'you really want to generate these automatically.  Then '
>                  'move them away.', ', '.join(already_exists))
> +        seen = [] # we throw this away right now, but could move template

Sentence should start with a capital.

> +        # processing up to __init__ if valuable.  That would mean that 
templates
> +        # would be rewritten even if a value in another section had been
> +        # referenced; however, it would also mean that __init__ would do
> +        # virtually all of the work, with install only doing the writing.
>          for rel_path, last_mod, st_mode in self.actions:
>              source = os.path.join(self.source_dir, rel_path)
>              dest = os.path.join(self.destination_dir, rel_path[:-3])
>              mode=stat.S_IMODE(st_mode)
> -            template=open(source).read()
> -            template=re.sub(r"\$\{([^:]+?)\}", r"${%s:\1}" % self.name,
> -                            template)
> -            self._create_paths(os.path.dirname(dest))
>              # we process the file first so that it won't be created if 
there
>              # is a problem.
> -            processed = self.options._sub(template, [])
> +            processed = Template(source).substitute(self, seen)
> +            self._create_paths(os.path.dirname(dest))
>              result=open(dest, "wt")
>              result.write(processed)
>              result.close()
> @@ -221,3 +230,74 @@
>  
>      def update(self):
>          pass
> +
> +
> +class Template:
> +    # hacked from string.Template
> +    pattern = re.compile(r"""
> +    \$(?:
> +      \${(?P<escaped>[^}]*)} |                   # Escape sequence of two 
delimiters.
> +      {(?P<braced_single>[-a-z0-9 ._]+)} |
> +                                         # Delimiter and a braced local 
option
> +      {(?P<braced_double>[-a-z0-9 ._]+:[-a-z0-9 ._]+)} |
> +                                         # Delimiter and a braced fully
> +                                         # qualified option (that is, with
> +                                         # explicit section).
> +      {(?P<invalid>[^}]*})               # Other ill-formed delimiter 
exprs.
> +    )
> +    """, re.IGNORECASE | re.VERBOSE)
> +
> +    def __init__(self, source):
> +        self.source = source
> +        self.template = open(source).read()
> +
> +    def _get_colno_lineno(self, i):
> +        lines = self.template[:i].splitlines(True)
> +        if not lines:
> +            colno = 1
> +            lineno = 1
> +        else:
> +            colno = i - len(''.join(lines[:-1]))
> +            lineno = len(lines)
> +        return colno, lineno
> +
> +    def _get(self, options, section, option, seen, start):
> +        value = options.get(option, None, seen)
> +        if value is None:
> +            colno, lineno = self._get_colno_lineno(start)
> +            raise zc.buildout.buildout.MissingOption(
> +                "Option '%s:%s', referenced in line %d, col %d of %s, "
> +                "does not exist." %
> +                (section, option, lineno, colno, self.source))
> +        return value
> +
> +    def substitute(self, recipe, seen):
> +        # Helper function for .sub()
> +        def convert(mo):
> +            # Check the most common path first.
> +            option = mo.group('braced_single')
> +            if option is not None:
> +                val = self._get(recipe.options, recipe.name, option, seen,
> +                                mo.start('braced_single'))
> +                # We use this idiom instead of str() because the latter 
will
> +                # fail if val is a Unicode containing non-ASCII characters.
> +                return '%s' % (val,)
> +            double = mo.group('braced_double')
> +            if double is not None:
> +                section, option = double.split(':')
> +                val = self._get(recipe.buildout[section], section, option, 
seen,
> +                                mo.start('braced_double'))
> +                return '%s' % (val,)
> +            escaped = mo.group('escaped')
> +            if escaped is not None:
> +                return '${%s}' % (escaped,)
> +            invalid = mo.group('invalid')
> +            if invalid is not None:
> +                colno, lineno = self._get_colno_lineno(mo.start('invalid'))
> +                raise ValueError(
> +                    'Invalid placeholder %r in line %d, col %d of %s' %
> +                    (mo.group('invalid'), lineno, colno, self.source))
> +            raise ValueError('Unrecognized named group in pattern',
> +                             self.pattern) # programmer error, AFAICT
> +        return self.pattern.sub(convert, self.template)
> +    

Trailing spaces.


> === modified file 'z3c/recipe/filetemplate/tests.py'
> --- z3c/recipe/filetemplate/tests.py	2009-04-30 17:56:10 +0000
> +++ z3c/recipe/filetemplate/tests.py	2009-07-09 18:20:56 +0000
> @@ -12,12 +12,24 @@
>  #
>  
##############################################################################
>  
> +import os
>  import zc.buildout.testing
>  import zc.buildout.tests
>  from zope.testing import doctest
>  
> +def write_and_wait(dir, *args):
> +    path = os.path.join(dir, *(args[:-1]))
> +    original = os.stat(path).st_mtime
> +    while os.stat(path).st_mtime == original:
> +        f = open(path, 'w')
> +        f.write(args[-1])
> +        f.flush()
> +        os.fsync(f.fileno())
> +        f.close()
> +

Why is this needed? Your CHANGES.txt file mentioned making tests more robust
against timing issues, but I don't see how that makes thing more robust?

I mean the mtime of path should be modified right after open(path, 'w'). I
don't see the rationale behind the loop, nor the necessity of f.flush() nor
fsysnc. Unless you are running things on a NFS-mounted volume... and even
there, I'm not sure it would help.

-- 
Francis J. Lacoste
<email address hidden>