BUG: test runner fails in Windows if filenames contain spaces.

[jdmarch]

In Windows test runner, inserts quotes around command line filenames with spaces when possible.
Workaround for #760 until #1888 is ready.

[takluyver]

Is there a reason we can't just replace os.system with subprocess.call and pass the arguments as a list?

[jdmarch]

@takluyver that should be fine; in particular, subprocess works as expected in #1888; this here was just a quick patch from a year ago, cherry-picked just now for comparing with #1888 failures.

[jdmarch]

@takluyver Actually, as-is I think would be safe for 0.13, since this is an extremely conservative PR. The change to subprocess is better in theory but is more fundamental, so should happen later.

[jdmarch]

Reinforcing my previous comment: when I change this PR to use subprocess, at least one additional test fails (IPython.core's "testing of issue ... 1107") referenced here.

[fperez]

I agree with going careful and conservative here, and delay a fundamentally better but possibly more disruptive change to subprocess for after the release. The code looks clean as-is to me; @takluyver, do you see any issues with merging this puppy?

[takluyver]

arg[0] != '"' checks if the argument is already quoted, but is it valid to quote arguments with 'single quotes'? Do we need to check for that as well?

Besides that, I think it's fine.

[jdmarch]

@takluyver It's a bit muddled, but I don't think we need to check for single quotes. A few observations:

• They are valid for arg quoting in gitbash, but not in cmd.exe
• They are valid filename characters in Windows.
• The above considerations are probably moot because I don't know how, in the context of the test runner, a single quote would ever slip into this argument list even in gitbash
• FWIW and disclosure: Actually, at this point I am not even sure that the check for double quote is necessary, because I do not see how, in the context of the test runner, a double quote would slip into the argument list either. I suspect that when I wrote this workaround last year, I put in the check for an existing double quote because I wasn't sure whether the double quotes that I was inserting might persist into a recursive invocation of the test runner. That doesn't seem to be the case, but I'll respect my qualms of last year and leave it in. I think it's clear that it does no harm.

[takluyver]

Thanks @jdmarch . I'm happy with this, then. I'll merge it now.