Correct string type casting in pinfo. #1178

Merged
merged 1 commit into from Dec 18, 2011

Projects

None yet

3 participants

@takluyver
Member

Haven't got time to work out a test at the moment, but this fixes #1177, and I don't believe it can introduce any failures that weren't already present.

Closes gh-1177

@minrk
Member
minrk commented Dec 18, 2011

Seems quite straightforward. I don't see any way for this to cause a problem, since the only case affected is bytes in on Python 2, which was already wrong. In all other cases, cast_bytes_py2 is identical to unicode_to_str.

A test would be great, but go ahead and merge if it's not feasible.

@takluyver
Member

It should be possible to do a test, but I'm not going to get round to it tonight. Since we've both come to the same conclusion about this, I'll merge it so it gets into 0.12, and open an issue to remind me to write a test.

@minrk
Member
minrk commented Dec 18, 2011

sounds good.

@takluyver takluyver merged commit a85c230 into ipython:master Dec 18, 2011
@fperez
Member
fperez commented Dec 18, 2011

OK, I agree. The change is clean and I also tested that with xlrd installed (ubuntu package python-xlrd), the bug is there in master and gets fixed by this. A simple test would be to create a temp file with cp1252 encoding that contains just

# coding: cp-1252
def f():
  "<problematic chars here>"

and then do in the test the equivalent of %run tempfile; get_ipython().inspector.info(f, detail_level=1).

But @takluyver, it's no problem if that test is added later on; go ahead and merge this even if you can't add the test now.

Note: @minrk made a good point about not rebasing even single-commit PRs, so that we can clearly see in the log those things that got reviewed and merged as PRs as opposed to direct commits. After that conversation, I've gotten into the habit of using the green button on this page, and simply copying (and editing for clarity as needed) the original PR message, including the closing directive.

@fperez
Member
fperez commented Dec 18, 2011

Well, you were faster than me :) Glad we were saying the same thing!

@takluyver
Member

On 18 December 2011 23:53, Fernando Perez <
reply@reply.github.com

wrote:

Note: @minrk made a good point about not rebasing even single-commit PRs,
so that we can clearly see in the log those things that got reviewed and
merged as PRs as opposed to direct commits. After that conversation, I've
gotten into the habit of using the green button on this page, and simply
copying (and editing for clarity as needed) the original PR message,
including the closing directive.

Ah, I'll remember that in future. I didn't rebase this, but there were no
more commits in master, so it fast-forwarded anyway.

@fperez
Member
fperez commented Dec 19, 2011

No worries; we've done a ton of even forced ff (via rebasing), so there's no big harm here. But the idea seemed sensible, so we can adopt it for the future.

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