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

Jump to bottom

Fix #9343: warn when using HTML instead of IFrame #11350

Merged
merged 1 commit into from Oct 3, 2018
Merged

Fix #9343: warn when using HTML instead of IFrame #11350

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions IPython/core/display.py
View file
Open in desktop
Expand Up @@ -666,6 +666,11 @@ def _repr_pretty_(self, pp, cycle):

class HTML(TextDisplayObject):

def __init__(self, data=None, url=None, filename=None, metadata=None):
if data and "<iframe " in data and "</iframe>" in data:
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if we should use startswith and endswith that are more efficient and likely more correct, but that looks good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I had the same thought. There’s a difference between rendering a single iframe vs rendering HTML with an iframe in it. Startswith would be a better idea. We could also account for case differences because HTML is case insensitive.

Let me know if you’d like those changes, or if things are fine as they are.

Copy link
Member

Choose a reason for hiding this comment

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

Up to you. I'll reopen the linked issue add link to this section for further improvements.

warnings.warn("Consider using IPython.display.IFrame instead")
super(HTML, self).__init__(data=data, url=url, filename=filename, metadata=metadata)
Copy link
Member

Choose a reason for hiding this comment

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

We're py3 only, so now you can just use super()


def _repr_html_(self):
return self._data_and_metadata()

Expand Down
8 changes: 8 additions & 0 deletions IPython/core/tests/test_display.py
View file
Open in desktop
Expand Up @@ -195,6 +195,14 @@ def test_displayobject_repr():
j._show_mem_addr = False
nt.assert_equal(repr(j), '<IPython.core.display.Javascript object>')

@mock.patch('warnings.warn')
def test_encourage_iframe_over_html(m_warn):
display.HTML('<br />')
m_warn.assert_not_called()
Copy link
Member

Choose a reason for hiding this comment

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

Ah I would have use assert_warns, but that looks good !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn’t know about that one, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had a look at the link that you posted, it seems specific to pytest, which IPython doesn't use. Did you mean to send this link? Let me know if I misunderstood something.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yes, sorry, and this I just get confused between test runners.


display.HTML('<iframe src="http://a.com"></iframe>')
m_warn.assert_called_with('Consider using IPython.display.IFrame instead')

def test_progress():
p = display.ProgressBar(10)
nt.assert_in('0/10',repr(p))
Expand Down