Shlex unicode #1116

Closed
wants to merge 3 commits into
from

3 participants

@jstenar
IPython member

Fix for issue #1115

@takluyver do you think this is the correct way to convert back to unicode?

Jörgen Stena... added some commits Dec 7, 2011
Jörgen Stenarson Fixing #1112 removing failing asserts for test_carriage_return and te…
…st_beep.

The failing asserts were checking the count property of the Action
tuple objects there is no count field in these Action tuples so
the count method of the regular tuple class is resolved.
8c6d953
Jörgen Stenarson Fixing shlex_split to return unicode on py2.x 3fcb420
@takluyver
IPython member

If we know that the output will always be in the native str type, we can use py3compat.str_to_unicode to do the conversion without requiring an if statement. Or have a look at the arg_split function @rkern mentioned on #1115.

Jörgen Stenarson Replaced shlex_split with arg_split from _process_common.
shlex_split was removed since it was a unicode unsafe version of
arg_split. Tests were added to test magic_run_completer.
52664ef
@takluyver
IPython member

@jstenar Unfortunately it looks like there's a merge conflict now. It's probably trivial, but if you've got time, could you rebase it.

@jstenar
IPython member
@minrk
IPython member

The conflict is just that the first commit has already been merged as a different PR, but GitHub can't tell. So, @takluyver if this looks fine to you, I will go ahead and merge with the necessary adjustments.

@takluyver
IPython member

The shlex_split function we're removing here handles exceptions with this trick involving removing the last character of the string. Have we worked out what that was achieving, and whether it needs to be preserved or replaced?

@minrk
IPython member

Ah, that's exactly what my PR #1130 does - change the behavior from raising, to just keeping the last blob.

So if that fix was ever necessary (I presume it was), then this doesn't work unless we also merge 1130.

@takluyver
IPython member

OK, that makes sense. And when that's merged, we should test this and make sure it gets the completions right in whatever situation it's handling.

@minrk minrk added a commit to minrk/ipython that referenced this pull request Dec 11, 2011
@minrk minrk add strict flag to arg_split, to optionally ignore shlex parse errors
Sometimes we pass things that aren't really command-line args to arg_split, e.g:

    %timeit python_code(" ")

This commit adds a `strict` flag, which defaults to the same raising behavior
as before.

Currently magic_timeit is the *only* place, we use strict=False, but it should
also be done in completions (PR #1116).

closes #1109
4f1e79b
@minrk
IPython member

I've now changed my code in #1130 to be more conservative (it takes a strict flag, which defaults to True, which is identical to the old behavior). After merging #1130, this code can use arg_split(s, strict=False), to preserve the error-ignoring behavior in shlex_split.

@minrk
IPython member

@takluyver - I actually don't see that we are getting any completions in the cases that this handles, even on master. It only serves to suppress the shlex ValueError. We don't complete "foo.py" from "fo, which I would consider a separate issue.

@takluyver
IPython member
@minrk
IPython member

Weird - it completes fine on Linux, but not OSX, so who knows what's up with that. In any case, it shouldn't be relevant to this PR.

I can merge #1130 and this (adding strict=False to match old behavior) if they look good to you, @takluyver.

@takluyver
IPython member

Assuming the "fo tab completion on Linux still works, go for it.

@minrk
IPython member

It does, tested both before and after the change.

Thanks! I'll go ahead and merge both.

@minrk minrk added a commit to minrk/ipython that referenced this pull request Dec 12, 2011
@minrk minrk Merge shlex PRs (#1130, #1116)
* arg_split now takes optional strict flag, to ignore ValueErrors in
  shlex parsing
* %timeit uses strict=False, to avoid errors parsing python code
* %run completer uses arg_split(strict=False) for its unicode behavior, instead
  of custom shlex derivative, which is now redundant.

closes #1109
closes #1115
closes #1116
closes #1130
790cb14
@minrk minrk added a commit that closed this pull request Dec 12, 2011
@minrk minrk Merge shlex PRs (#1130, #1116)
* arg_split now takes optional strict flag, to ignore ValueErrors in
  shlex parsing
* %timeit uses strict=False, to avoid errors parsing python code
* %run completer uses arg_split(strict=False) for its unicode behavior, instead
  of custom shlex derivative, which is now redundant.

closes #1109
closes #1115
closes #1116
closes #1130
790cb14
@minrk minrk closed this in 790cb14 Dec 12, 2011
@mattvonrocketstein mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
@minrk minrk add strict flag to arg_split, to optionally ignore shlex parse errors
Sometimes we pass things that aren't really command-line args to arg_split, e.g:

    %timeit python_code(" ")

This commit adds a `strict` flag, which defaults to the same raising behavior
as before.

Currently magic_timeit is the *only* place, we use strict=False, but it should
also be done in completions (PR #1116).

closes #1109
52b3af3
@mattvonrocketstein mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
@minrk minrk Merge shlex PRs (#1130, #1116)
* arg_split now takes optional strict flag, to ignore ValueErrors in
  shlex parsing
* %timeit uses strict=False, to avoid errors parsing python code
* %run completer uses arg_split(strict=False) for its unicode behavior, instead
  of custom shlex derivative, which is now redundant.

closes #1109
closes #1115
closes #1116
closes #1130
5799471
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment