pexpect & Python 3 #798

Merged
merged 3 commits into from Sep 29, 2011

Conversation

Projects
None yet
3 participants
@takluyver
Member

takluyver commented Sep 16, 2011

Changes to pexpect so running external processes from the Qt console works in Python 3.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Sep 17, 2011

Member

These look fine to merge, but I'd suggest you split it into two separate commits. Since one entails changes to an external dependency, I think it's best to always have those available in nice, isolated commits. It makes it easier to send the patch upstream, reapply it if we update to a new version from upstream that doesn't have the changes, etc.

So my vote is: rebase by splitting into two commits, one for each file, then merge away.

Thanks!

Member

fperez commented Sep 17, 2011

These look fine to merge, but I'd suggest you split it into two separate commits. Since one entails changes to an external dependency, I think it's best to always have those available in nice, isolated commits. It makes it easier to send the patch upstream, reapply it if we update to a new version from upstream that doesn't have the changes, etc.

So my vote is: rebase by splitting into two commits, one for each file, then merge away.

Thanks!

takluyver added some commits Sep 17, 2011

Try locale encoding if stdin encoding is ascii.
Starting the Qt console on Python 3, the kernel's stdin ends up with a .encoding of 'ascii' (whereas on Python 2 it is None). Since most platforms can handle a superset of ASCII, we may as well try locale.getpreferredencoding() in this case.
@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Sep 17, 2011

Member

OK, I've split them, but I found another minor bug. The kernel in Python 2 has a sys.stdin.encoding of None, so @minrk's getdefaultencoding() function fell back to using the locale. In Python 3, sys.stdin.encoding is 'ascii', although the system is using UTF-8.

On the principle that most systems now can handle a superset of ascii (either utf-8 or a Windows code page), I've made getdefaultencoding() try the locale encoding if sys.stdin.encoding is None or ascii. I think the only way this could make anything worse is if a subprocess is returning ascii output on a system where the locale encoding is not ascii compatible (e.g. UTF-16).

Member

takluyver commented Sep 17, 2011

OK, I've split them, but I found another minor bug. The kernel in Python 2 has a sys.stdin.encoding of None, so @minrk's getdefaultencoding() function fell back to using the locale. In Python 3, sys.stdin.encoding is 'ascii', although the system is using UTF-8.

On the principle that most systems now can handle a superset of ascii (either utf-8 or a Windows code page), I've made getdefaultencoding() try the locale encoding if sys.stdin.encoding is None or ascii. I think the only way this could make anything worse is if a subprocess is returning ascii output on a system where the locale encoding is not ascii compatible (e.g. UTF-16).

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Sep 29, 2011

Member

I'll merge this soon unless anyone objects.

Member

takluyver commented Sep 29, 2011

I'll merge this soon unless anyone objects.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Sep 29, 2011

Member

seems fine to me.

Do you know why Python3 incorrectly marks sys.stdin.encoding as ascii? Or is that the new replacement for None? The problem is that if the terminal really is ASCII, we might run into problems. But I wouldn't worry too much about that.

Member

minrk commented Sep 29, 2011

seems fine to me.

Do you know why Python3 incorrectly marks sys.stdin.encoding as ascii? Or is that the new replacement for None? The problem is that if the terminal really is ASCII, we might run into problems. But I wouldn't worry too much about that.

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Sep 29, 2011

Member

I think that the object which is now used for sys.stdin (io.TextIOWrapper) has to have a non-None encoding attribute, so ascii is the default if it's not told anything else.

I don't think it should cause problems, because:

  1. If the terminal is ASCII, the default locale should presumably indicate ascii as well.
  2. If the terminal is ASCII and the default locale indicates another encoding, it will most likely be either UTF-8 or an ascii compatible code page. So any characters we get from the terminal should be correctly decoded anyway. The only situation in which it could is if the locale incorrectly indicates a non-ascii-compatible encoding, such as UTF-16.
Member

takluyver commented Sep 29, 2011

I think that the object which is now used for sys.stdin (io.TextIOWrapper) has to have a non-None encoding attribute, so ascii is the default if it's not told anything else.

I don't think it should cause problems, because:

  1. If the terminal is ASCII, the default locale should presumably indicate ascii as well.
  2. If the terminal is ASCII and the default locale indicates another encoding, it will most likely be either UTF-8 or an ascii compatible code page. So any characters we get from the terminal should be correctly decoded anyway. The only situation in which it could is if the locale incorrectly indicates a non-ascii-compatible encoding, such as UTF-16.
@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Sep 29, 2011

Member

Sounds good.

Member

minrk commented Sep 29, 2011

Sounds good.

takluyver added a commit that referenced this pull request Sep 29, 2011

Merge pull request #798 from takluyver/py3-pexpect
Minimal fixes for pexpect on Python 3, so we can run processes from ZMQ frontends.

@takluyver takluyver merged commit 5590396 into ipython:master Sep 29, 2011

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Sep 29, 2011

Member

Thanks, Min. Merged.

Member

takluyver commented Sep 29, 2011

Thanks, Min. Merged.

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request #798 from takluyver/py3-pexpect
Minimal fixes for pexpect on Python 3, so we can run processes from ZMQ frontends.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment