Skip to content
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.

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

Win32 shlex #1064

Merged
merged 7 commits into from Nov 30, 2011
Merged

Win32 shlex #1064

merged 7 commits into from Nov 30, 2011

Conversation

jstenar
Copy link
Member

@jstenar 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
Copy link
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.

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']],
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Member

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

@fperez
Copy link
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
Copy link
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
Copy link
Member Author

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
Copy link
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
Copy link
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
Copy link
Member Author

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
Copy link
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
Copy link
Member

Great. Tested and merged. Thanks!

This was referenced Dec 13, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants