No longer concatenating extensions #2381
Conversation
| cmd = 'dot -T%s -O \'%s\'' % (format, dotfilename) | ||
| dot_base = os.path.splitext(dotfilename)[0] | ||
| formatted_dot = '{}.{}'.format(dot_base, format) | ||
| cmd = 'dot -T{} -o"{}" "{}"'.format(format, formatted_dot, dotfilename) |
There was a problem hiding this comment.
I think this needs to be uppercase O
There was a problem hiding this comment.
nope, -ofile is what we should be using.
| cmd = 'dot -T%s -O \'%s\'' % (format, dotfilename) | ||
| dot_base = os.path.splitext(dotfilename)[0] | ||
| formatted_dot = '{}.{}'.format(dot_base, format) | ||
| cmd = 'dot -T{} -o"{}" "{}"'.format(format, formatted_dot, dotfilename) |
There was a problem hiding this comment.
Returning formatted_dot breaks if format == 'dot'. One option is to revert the return line, and update these three lines to:
dot_base = os.path.splitext(dotfilename)[0]
orig_dot = dotfilename
dotfilename = '{}.{}'.format(dot_base, format)
cmd = 'dot -T{} -o"{}" "{}"'.format(format, dotfilename, orig_dot)There was a problem hiding this comment.
thanks for the advice, but I would recommend the current approach. Having dotfilename mean both the original and the formatted filename is confusing, and I would rather not construct the formatted name implicitly inline with some other operation for the sake of clarity.
If the filename for some reason can't be split, I'd much rather like the traceback to serve me up this line directly:
dot_base = os.path.splitext(dotfilename)[0]
There was a problem hiding this comment.
The issue is that you're returning a value from an unassigned name in the case where format == 'dot'. If you don't want to reassign a variable, you can return the formatted_dot from within the if block, or return dotfilename if format == 'dot' else formatted_dot, or whatever, but the tests will need to pass before we can merge this.
graph.png and graph.dot side by side is a lot better than graph.dot.png and graph.dot - for both tab completion, and software which has trouble intelligently parsing extensions (e.g. LaTeX's \includegraphics).
reiteration of #2268