Skip to content

No longer concatenating extensions #2381

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 21, 2018
Merged

No longer concatenating extensions #2381

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions nipype/pipeline/engine/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1314,18 +1314,19 @@ def export_graph(graph_in,
def format_dot(dotfilename, format='png'):
"""Dump a directed graph (Linux only; install via `brew` on OSX)"""
if format != 'dot':
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to be uppercase O

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope, -ofile is what we should be using.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator Author

@TheChymera TheChymera Jan 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

try:
CommandLine(cmd, resource_monitor=False).run()
except IOError as ioe:
if "could not be found" in str(ioe):
raise IOError(
"Cannot draw directed graph; executable 'dot' is unavailable"
)
raise IOError("Cannot draw directed graph; executable 'dot' is unavailable")
else:
raise ioe
dotfilename += '.%s' % format
return dotfilename
else:
formatted_dot = dotfilename
return formatted_dot


def get_all_files(infile):
Expand Down
3 changes: 2 additions & 1 deletion tools/interfacedocgen.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,8 @@ def _write_graph_section(self, fname, title):

fhandle.close()
os.remove(fname)
os.remove(fname + ".png")
bitmap_fname = '{}.png'.format(os.path.splitext(fname)[0])
os.remove(bitmap_fname)
return ad

def generate_api_doc(self, uri):
Expand Down