Win32 shlex #1064

Merged
merged 7 commits into from Nov 30, 2011

Projects

None yet

3 participants

@jstenar
Member
jstenar commented Nov 28, 2011

Suggested more complete fix for issue #592. Using ctypes to call a windows function for doing shlex like splitting.

Had to comment out the unicode strings in test_arg_split to get the tests to run (see #1063).

@takluyver
Member

Note that we've got modules in utils named _process_win32 and _process_posix. That's probably the place to put platform specific logic like this.

@takluyver takluyver and 1 other commented on an outdated diff Nov 28, 2011
IPython/utils/tests/test_process.py
def test_arg_split():
"""Ensure that argument lines are correctly split like in a shell."""
tests = [['hi', ['hi']],
[u'hi', [u'hi']],
['hello there', ['hello', 'there']],
- [u'h\N{LATIN SMALL LETTER A WITH CARON}llo', [u'h\N{LATIN SMALL LETTER A WITH CARON}llo']],
+# [u'h\N{LATIN SMALL LETTER A WITH CARON}llo', [u'h\N{LATIN SMALL LETTER A WITH CARON}llo']],
@takluyver
takluyver Nov 28, 2011 IPython member

I'd rather not comment out this test. I don't think it's essential to use a name escape: it should work with a \u escape.

@jstenar
jstenar Nov 28, 2011 IPython member

Thomas skrev 2011-11-29 00:08:

def test_arg_split():
"""Ensure that argument lines are correctly split like in a shell."""
tests = [['hi', ['hi']],
[u'hi', [u'hi']],
['hello there', ['hello', 'there']],

  •         [u'h\N{LATIN SMALL LETTER A WITH CARON}llo', [u'h\N{LATIN SMALL LETTER A WITH CARON}llo']],
    
    +# [u'h\N{LATIN SMALL LETTER A WITH CARON}llo', [u'h\N{LATIN SMALL LETTER A WITH CARON}llo']],

I'd rather not comment out this test. I don't think it's essential to use a name escape: it should work with a \u escape.


Reply to this email directly or view it on GitHub:
https://github.com/ipython/ipython/pull/1064/files#r256132
It was never my intention to keep them commented out. I just did it to
get the tests running at all. I'll switch them over to \u escapes.

/Jörgen

@takluyver
Member

Do we need a pure-python fallback? Is there any situation in which all this ctypes stuff could fail?

@fperez
Member
fperez commented Nov 29, 2011

@takluyver, you're our unicode guru; is this good to go or does it need any further work?

@takluyver
Member

Well, it doesn't really change anything on the posix side (I'm assuming Jörgen just copied and pasted the arg_split function for the posix version, I haven't checked it character-for-character). The windows ctypes stuff doesn't mean much to me.

@jstenar: I'd just like to clarify: is there any version of Windows, any locale, or other setting, under which it could fail? I know that ctypes code can cause segfaults if it goes wrong, and I can't test it here. Also, is there any way to access this functionality through a library like pywin32 - even if we have this as a fallback when that's not installed? I'm a bit wary of relying on ctypes code.

@jstenar
Member
jstenar commented Nov 30, 2011

As far as I know CommandLineToArgvW is not dependent on locales. It is however available only from Windows 2000 and forward. I could add a try/except guard to catch if it is missing and fall back to the old posix implementation (by copying it, because I can't import _process_posix on windows).

I could not find this function in pywin32.

@takluyver
Member

That sounds OK. I doubt we really need to worry about Windows 98 support at
this point.

There's also _process_common, where you can put stuff to be imported by
_process_posix and _process_win32. I'll leave it to your judgement whether
you want to add a try/except for it

@fperez
Member
fperez commented Nov 30, 2011

We're definitely not worrying about windows 98! XP and newer is more than enough of a cutoff, I think.

@jstenar
Member
jstenar commented Nov 30, 2011

I moved the posix version of arg_split to _process_common and use that as a fallback in _process_win32 if CommandLineToArgvW. That way we will have a fallback and not just crash.

@fperez
Member
fperez commented Nov 30, 2011

Great, thanks! @takluyver, this is looking pretty baked out then, right? If both you and @jstenar are happy with it, merge away! @jstenar, thanks for the work :)

ps - double-check that any issues supposed to be closed do get closed, I've seen github recently flake out and not closing issues listed in commits.

@takluyver takluyver merged commit c72bbc7 into ipython:master Nov 30, 2011
@takluyver
Member

Great. Tested and merged. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment