fix: logging error if % in node cmd #2364
Conversation
| message += ', a CommandLine Interface with command:\n%s' % cmd | ||
| logger.info(message, self.name, self._interface.__module__, | ||
| self._interface.__class__.__name__) | ||
| message += ', a CommandLine Interface with command:\n{}'.format(cmd) |
There was a problem hiding this comment.
why not cmd.replace('%', '%%')?
There was a problem hiding this comment.
Actually, this problem now will happen the same if the command has braces...
There was a problem hiding this comment.
good call - I would prefer to not alter the cmd if possible, what about substitution first?
There was a problem hiding this comment.
cmd.replace('%', '%%') is not an in-place operation, why not just do it here?
There was a problem hiding this comment.
sure, we can replace to cover against substitution (%%) / escape characters (\", \\, etc), but isn't .format cleaner?
There was a problem hiding this comment.
It is just to use python's logging standards, but I'm not particularly attached to one solution or another 👍
nipype/pipeline/engine/nodes.py
Outdated
| @@ -577,7 +577,9 @@ def _run_command(self, execute, copyfiles=True): | |||
| self._originputs = deepcopy(self._interface.inputs) | |||
| self._copyfiles_to_wd(execute=execute) | |||
|
|
|||
| message = '[Node] Running "{}" ("{}.{}")' | |||
| message = '[Node] Running "{}" ("{}.{}")'.format( | |||
There was a problem hiding this comment.
None of these variables should contain percent characters. Why not letting the logging module do the work?
There was a problem hiding this comment.
Better to do the substitution first (before cmd is added), since a rouge % or {} will lead to a logging error
Fixes #2325 (comment).
Changes proposed in this pull request
.formatsubstitutionEventually, we should be thorough and replace most
%substitutions