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

protect against broken repr in lib.pretty #4459

Merged
merged 4 commits into from Nov 4, 2013
Merged

Conversation

minrk
Copy link
Member

@minrk minrk commented Oct 30, 2013

for example:

from lxml import html
import urllib2
from IPython.display import display

term = 'cat'
data = html.parse("""http://en.wiktionary.org/w/index.php?title=%s&printable=yes""" % urllib2.quote(term.encode('utf-8')), parser=html.HTMLParser(encoding="utf-8"))
d = data.xpath(".//div[@id='mw-content-text']")[0]
display(d.getchildren())

try:
return repr(obj)
except Exception as e:
return "<repr(obj) failed: %s>" % e
Copy link
Contributor

Choose a reason for hiding this comment

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

I find it useful to include the type of the exception as well.

return "<repr(obj) failed: {0}: {1}>".format(type(e), e)

Copy link
Contributor

Choose a reason for hiding this comment

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

While I agree that reporting the exception message is useful, can we still at least include the usual <module.type at 0x.....> information? That will help locate the source of the problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

type info / address added, along with some extra paranoia about bad objects

don't trust exception strings or class attributes.

adds _safe_getattr, used in a few higher level places to protect against bad objects
@rkern
Copy link
Contributor

rkern commented Oct 30, 2013

I like it. The docstring on _safe_getattr could be cleaned up, but otherwise LGTM.

@minrk
Copy link
Member Author

minrk commented Oct 30, 2013

cleaned up the docstring a bit, thanks.

@takluyver
Copy link
Member

For the record, there are a couple of pathological cases which could still fail. Exceptions inheriting from BaseException directly won't be caught - that's kind of the design of the system, to allow KeyboardInterrupt and SystemExit to keep propagating when you'd catch any other exception. And on Python 2, old style classes from outside the exception hierarchy can still be raised. However, all of the exceptions Python itself uses are in the hierarchy, so that's only possible if something is called than does an explicit raise.

I don't think we need to worry about either of those cases, I just wanted to mention it in case anyone disagrees.

@takluyver
Copy link
Member

Merging this.

takluyver added a commit that referenced this pull request Nov 4, 2013
protect against broken repr in lib.pretty
@takluyver takluyver merged commit 49af4ae into ipython:master Nov 4, 2013
@minrk minrk deleted the bad-repr branch March 31, 2014 23:36
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
protect against broken repr in lib.pretty
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