Code review comment for lp:~gary/z3c.recipe.filetemplate/support-system-python

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>