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

Revision history for this message
Francis J. Lacoste (flacoste) wrote :

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.

> === 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?
> - 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.

> +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?

review: Approve

« Back to merge proposal