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

Fix for #1688, traceback-unicode issue #1715

Merged
merged 35 commits into from Sep 6, 2012

Conversation

jstenar
Copy link
Member

@jstenar jstenar commented May 9, 2012

PR to work on a fix for #1688. Right now there is one commit with a quick and dirty solution using unicode_literals from __future__ and two casts to unicode. No new test failures were noted.

There are more locations where we will have errors. Any calls to pycolorize needs to make sure to use only unicode. One example is docstring printing using ?

I could add casts in pycolorize instead, should I do that?

@@ -69,7 +69,7 @@
# the file COPYING, distributed as part of this software.
#*****************************************************************************

from __future__ import with_statement
from __future__ import with_statement, unicode_literals
Copy link
Member

Choose a reason for hiding this comment

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

We can get rid of with_statement here - we already dropped Python 2.5 support, and it's native in 2.6+.

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 Kluyver skrev 2012-05-09 21:38:

We can get rid of with_statement here - we already dropped Python 2.5 support, and it's native in 2.6+.

ok I'll remove it

@takluyver
Copy link
Member

We need to be a little bit careful with unicode_literals, because that converts every string in the file, which can have unintended side effects. I've had a scan down the file, and there are no obvious problems, but I'd like us to do some manual testing of this before it lands, because these modules are poorly covered by automated tests.

@takluyver
Copy link
Member

Test results for commit ca15c23 merged into master
Platform: linux2

  • python2.7: OK
  • python3.1: OK (libraries not available: matplotlib pymongo qt wx wx.aui zmq)
  • python3.2: OK (libraries not available: pymongo wx wx.aui)

Not available for testing: python2.6

@takluyver
Copy link
Member

For manual testing, three commands I'd like to check work:

%pycat foo.py   # use any reasonably complex Python file you have to hand
1/0                   # trigger a regular traceback
raise = 2           # and a SyntaxError traceback

The focus is on Python 2, since this shouldn't change anything for Python 3. Also, cycle through the three traceback modes using %xmode and repeat the last two tests.

@takluyver
Copy link
Member

It all seems to be OK on Linux, with Python 2.7.3 and with 2.6.7.

@jstenar
Copy link
Member Author

jstenar commented May 10, 2012

In magic_paste text is sent to colorize from the clipboard. What happens on non-windows platforms? Will clipboard_get return a str or unicode. If str what should I assume about encoding?

@takluyver
Copy link
Member

I think some of the new functionality in IPython.utils.io is redundant with stuff in IPython.utils.openpy - can you look into merging it into that module?

This PR has also picked up a conflict with master, so we'll need to rebase it before we can merge.

@takluyver
Copy link
Member

clipboard_get() on Linux appears to retrieve misformed unicode, it looks like this is a bug in Tkinter. On OS X, it's calling a subprocess, so it will likely be in bytes; that should really be fixed within clipboard_get.

@takluyver
Copy link
Member

I've filed a Python issue for the problems with Tkinter clipboard_get(): http://bugs.python.org/issue14777

@jstenar
Copy link
Member Author

jstenar commented May 11, 2012

I'll look into it. I'm travelling so it will not be before sunday.

11 maj 2012 kl. 00:21 skrev Thomas Kluyver reply@reply.github.com:

I think some of the new functionality in IPython.utils.io is redundant with stuff in IPython.utils.openpy - can you look into merging it into that module?

This PR has also picked up a conflict with master, so we'll need to rebase it before we can merge.


Reply to this email directly or view it on GitHub:
#1715 (comment)

@jstenar
Copy link
Member Author

jstenar commented May 14, 2012

Thomas Kluyver skrev 2012-05-11 00:21:

I think some of the new functionality in IPython.utils.io is redundant with stuff in IPython.utils.openpy - can you look into merging it into that module?

This PR has also picked up a conflict with master, so we'll need to rebase it before we can merge.

I have done a rebase

@jstenar
Copy link
Member Author

jstenar commented May 14, 2012

Thomas Kluyver skrev 2012-05-11 00:31:

clipboard_get() on Linux appears to retrieve misformed unicode, it looks like this is a bug in Tkinter. On OS X, it's calling a subprocess, so it will likely be in bytes; that should really be fixed within clipboard_get.

I have added casts to unicode for clipboard_get on Linux and OSX. I used
DEFAULT_ENCODING.

@jstenar
Copy link
Member Author

jstenar commented May 15, 2012

How do I use the demo mode? In the docs there is a reference to docs/examples/core/demo-exercizer.py but that file seems to be gone.

I think demo mode is broken with this PR as is but I do not know how to use it.

@takluyver
Copy link
Member

@@ -14,6 +14,7 @@
#-----------------------------------------------------------------------------
# Imports
#-----------------------------------------------------------------------------
import re
Copy link
Member

Choose a reason for hiding this comment

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

This is added with no other changes to the file - I guess it's a leftover?

@jstenar
Copy link
Member Author

jstenar commented May 15, 2012

Thomas Kluyver skrev 2012-05-15 22:16:

Is demo mode the same thing as this? http://ipython.org/ipython-doc/dev/interactive/reference.html#interactive-demos-with-ipython

yes that description should be enough. I found this one
http://ipython.org/ipython-doc/rel-0.12.1/api/generated/IPython.lib.demo.html?highlight=demo#IPython.lib.demo

@takluyver
Copy link
Member

I've got the problem with Tkinter fixed for Python 3.3/3.2.4/2.7.4. I'm now a CPython contributor! :-)

@jstenar
Copy link
Member Author

jstenar commented May 31, 2012

I just rebased from master 2caea25 after the magics PR.

@jstenar
Copy link
Member Author

jstenar commented May 31, 2012

The magics refactoring broke several magic commands (pdef, pdoc, psource, pfile) commit a71d86a fixes this

@@ -361,9 +373,8 @@ def format_stack_entry(self, frame_lineno, lprefix=': ', context = 3):
and tpl_line_em \
or tpl_line
ret.append(self.__format_line(linetpl, filename,
start + 1 + i, line,
start + 1 + i, line.decode(encoding, errors="replace"),
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure about this? I expect in Python 3, line will already be str, so decoding it again will fail.

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 Kluyver skrev 2012-05-31 21:46:

@@ -361,9 +373,8 @@ def format_stack_entry(self, frame_lineno, lprefix=': ', context = 3):
and tpl_line_em
or tpl_line
ret.append(self.__format_line(linetpl, filename,

  •                                      start + 1 + i, line,
    
  •                                      start + 1 + i, line.decode(encoding, errors="replace"),
    

Are you sure about this? I expect in Python 3, line will already be str, so decoding it again will fail.

fixed in 98fd6f8

@takluyver
Copy link
Member

Test results for commit a71d86a merged into master
Platform: linux2

  • python2.7: OK
  • python3.1: OK (libraries not available: cython matplotlib pymongo qt wx wx.aui zmq)
  • python3.2: OK (libraries not available: cython pymongo wx wx.aui)

Not available for testing: python2.6

@fperez
Copy link
Member

fperez commented Jun 10, 2012

Hey guys, just touching bases on this. How far do you think we are from merging it? We have so many PRs in flight that I'm a bit worried and don't want to create multiple conflicts everywhere... We also need to begin thinking about 0.13 sometime soon...

@@ -351,6 +367,10 @@ def format_stack_entry(self, frame_lineno, lprefix=': ', context = 3):

start = lineno - 1 - context//2
lines = linecache.getlines(filename)
try:
encoding, _ = openpy.detect_encoding(_readline(lines))
Copy link
Member

Choose a reason for hiding this comment

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

I suspect Python 3 linecache will take care of decoding lines for us, and this will go wrong.

Copy link
Member

Choose a reason for hiding this comment

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

The test run appears to confirm my suspicion.

@travisbot
Copy link

This pull request passes (merged ca43b98 into 011222a).

Most files in linecache are stored with the filename as a bytestring
(py2.x). When calling getlines with a unicode filename there will be
implicit conversions using ascii that fails. This is avoided by an
explicit conversion using the filesystem encoding before calling
getlines.
@travisbot
Copy link

This pull request passes (merged 50c631d into 011222a).

For instance Windows IO error messages are localized and may contain
non-ascii chracters.
@travisbot
Copy link

This pull request passes (merged 8ed4178 into 011222a).

@takluyver
Copy link
Member

Test results for commit 8ed4178 merged into master (c678a87)
Platform: linux2

  • python2.7: OK (libraries not available: azure oct2py pymongo wx wx.aui)
  • python3.2: OK (libraries not available: azure oct2py pymongo wx wx.aui)

Extra args: ['--all']
Not available for testing: python2.6

@takluyver
Copy link
Member

OK, we reviewed this at Euroscipy and fixed things up. I think it should go in so we can start finding any odd consequences. @jstenar @fperez @ellisonbg @minrk @bfroehle , I'll merge this tomorrow if no-one objects.

jstenar added a commit that referenced this pull request Sep 6, 2012
@jstenar jstenar merged commit 5d645a2 into ipython:master Sep 6, 2012
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Add git-mpr, to merge PR(s) from github just by number(s).

Inspired by git-mrb and test_pr, I thougth it would be usefull to be able to merge PR only by giving their number. Hence `git merge-pull-request`or `git-mpr`.

usage: 
```bash
$ git mpr -h                                                                          ~/ipython/tools
usage: git-mpr [-h] [-l | -a | -m [pr-number [pr-number ...]]]

Merge (one|many) github pull request by their number. If pull request can't be
merge as is, cancel merge, and continue to the next if any.

optional arguments:
  -h, --help            show this help message and exit
  -l, --list            list PR, their number and their mergeability
  -a, --merge-all       try to merge as many PR as possible, one by one
  -m [pr-number [pr-number ...]], --merge [pr-number [pr-number ...]]
                        The pull request numbers
```

examples :
```bash
$ git mpr --list  
* ipython#1758 [√]:  test_pr, fallback on http if git protocol fail, and SSL errors...
* ipython#1755 [√]:  test for pygments before running qt tests
* ipython#1715 [√]:  Fix for ipython#1688, traceback-unicode issue
[...]
* ipython#1493 [√]:  Selenium web tests proof-of-concept
* ipython#1471 [ ]:  simplify IPython.parallel connections and enable Controller Resume
* ipython#1343 [ ]:  Add prototype shutdown button to Notebook dashboard
* ipython#1285 [√]:  Implementation of grepping the input history using several patterns
* ipython#1215 [√]:  updated %quickref to show short-hand for %sc and %sx
```
PR number, mergeability and title
Quite slow, as it does 1 api call by PR, since api does not give mergeability anymore if you ask for the list of all PRs at once.

merge one or more PR (skip the ones with conflict and present a nice list to copy and past to do it by hand) 

```bash
$ git mpr --merge [pr-number [pr-number ...]]]
[...]
*************************************************************************************
the following branch have not been merged automatically, considere doing it by hand :
PR 1630: git pull https://github.com/minrk/ipython.git mergekernel
PR 1343: git pull https://github.com/takluyver/ipython.git notebook-shutdown
PR 1715: git pull https://github.com/jstenar/ipython.git ultratb-pycolorize-unicode
PR 1732: git pull https://github.com/fperez/ipython.git cellmagics
PR 1471: git pull https://github.com/minrk/ipython.git connection
PR 1674: git pull https://github.com/mdboom/ipython.git notebook-carriage-return
*************************************************************************************
```

And last, 
```
git mpr --merge-all
```

That is pretty self explainatory
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
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

6 participants