Code review comment for lp://staging/~gary/z3c.recipe.filetemplate/relative-paths

On Apr 19, 2010, at 4:28 PM, Francis J. Lacoste wrote:

> Review: Approve
> I have a couple of minor comments and questions. Should be good to go.
>
>> === modified file 'z3c/recipe/filetemplate/README.txt'
>
>> + >>> cat(sample_buildout, 'bin', 'dosomething.sh') # doctest: +ELLIPSIS
>> + #!/bin/sh
>> + Z3C_RECIPE_FILETEMPLATE_BASE=`\
>> + readlink -f "$0" 2>/dev/null || \
>> + realpath "$0" 2>/dev/null || \
>> + type -P "$0" 2>/dev/null`
>> + Z3C_RECIPE_FILETEMPLATE_BASE=`dirname ${Z3C_RECIPE_FILETEMPLATE_BASE}`
>> + Z3C_RECIPE_FILETEMPLATE_BASE=`dirname ${Z3C_RECIPE_FILETEMPLATE_BASE}`
>> + cat "$Z3C_RECIPE_FILETEMPLATE_BASE"/data/info.csv
>> +
>
> The multiple mutation of Z3C_RECIPE_FILETEMPLATE_BASE are confusing.
> Especially, the two identical Z3C_RECIPE_FILETEMPLATE_BASE=`dirname ...`
> I assume that's the way to get to the parent directory of the script. Would be
> clearer to use multiple variables, at the detriment of more namespace
> polluting. At the very least a comment would help the reader. Your call.

Done, with variable name changes and comments, for both shell and Python versions.

>
>> === modified file 'z3c/recipe/filetemplate/__init__.py'
>
>> + def _get(self, section, option, start):
>> + if section is None:
>> + section = self.recipe.name # This sets up error messages properly.
>> + if section == self.recipe.name:
>> + factory = self.recipe.dynamic_options.get(option)
>> + if factory is not None:
>> + try:
>> + return factory(self, start, option)
>> + except:
>> + # Argh. Would like to raise wrapped exception.
>> + colno, lineno = self.get_colno_lineno(start)
>> + msg = ('Dynamic option %r in line %d, col %d of %s '
>> + 'crashed.') % (option, lineno, colno, self.source)
>> + self.recipe.logger.error(msg, exc_info=True)
>> + raise
>> + # else...
>> + options = self.recipe.options
>>
>
> Shouldn't you catch and re-raise SystemExit and KeyboardInterrupt? Or was this
> fixed in Python 2.5?

Changed.

>> - def substitute(self, recipe, seen):
>> + def substitute(self):
>> def convert(mo):
>> + start = mo.start()
>> # Check the most common path first.
>> - option = mo.group('braced_single')
>> + option = mo.group('option')
>> if option is not None:
>> - val = self._get(recipe.options, recipe.name, option, seen,
>> - mo.start('braced_single'))
>> + section = mo.group('section')
>> + val = self._get(section, option, start)
>> + path_extension = mo.group('path_extension')
>> + filters = mo.group('filters')
>> + if path_extension is not None:
>> + val = os.path.join(val, *path_extension.split('/')[1:])
>> + if filters is not None:
>> + for filter in filters.split('|')[1:]:
>> + filter = filter.strip()
>> + if filter not in self.recipe.filters:
>> + colno, lineno = self.get_colno_lineno(start)
>> + raise ValueError(
>> + 'Unknown filter %r '
>> + 'in line %d, col %d of %s' %
>> + (filter, lineno, colno, self.source))
>> + try:
>> + val = self.recipe.filters[filter](
>> + val, self, start, filter)
>> + except:
>> + # Argh. Would like to raise wrapped exception.
>> + colno, lineno = self.get_colno_lineno(start)
>> + msg = ('Filter %r in line %d, col %d of %s '
>> + 'crashed processing value %r') % (
>> + filter, lineno, colno, self.source, val)
>> + self.recipe.logger.error(msg, exc_info=True)
>> + raise
>
> Same question about the bare except.

Changed.

>
>> +def _relative_depth(common, path):
>> + # Helper ripped from zc.buildout.easy_install.
>
> Is the copy and paste really necessary? easy_install isn't something that can
> be imported?

easy_install can be imported, but this is also a protected (_*) name in that module, so it cannot be safely imported, by convention.

Gary