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 #9343: warn when using HTML instead of IFrame #11350

Merged
merged 1 commit into from Oct 3, 2018

Conversation

Projects
None yet
2 participants
@mpenkov
Contributor

mpenkov commented Oct 1, 2018

No description provided.

def __init__(self, data=None, url=None, filename=None, metadata=None):
if data and "<iframe " in data and "</iframe>" in data:
warnings.warn("Consider using IPython.display.IFrame instead")
super(HTML, self).__init__(data=data, url=url, filename=filename, metadata=metadata)

This comment has been minimized.

@Carreau

Carreau Oct 3, 2018

Member

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

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

This comment has been minimized.

@Carreau

Carreau Oct 3, 2018

Member

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

This comment has been minimized.

@mpenkov

mpenkov Oct 3, 2018

Contributor

Didn’t know about that one, thanks!

This comment has been minimized.

@mpenkov

mpenkov Oct 7, 2018

Contributor

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.

This comment has been minimized.

@Carreau

Carreau Oct 9, 2018

Member

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

@@ -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:

This comment has been minimized.

@Carreau

Carreau Oct 3, 2018

Member

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

This comment has been minimized.

@mpenkov

mpenkov Oct 3, 2018

Contributor

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.

This comment has been minimized.

@Carreau

Carreau Oct 3, 2018

Member

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

@Carreau

This comment has been minimized.

Member

Carreau commented Oct 3, 2018

Thanks !

@Carreau Carreau merged commit 58bfd4f into ipython:master Oct 3, 2018

4 checks passed

codecov/patch 100% of diff hit (target 0%)
Details
codecov/project 68.2% (+<.01%) compared to 194d1d7
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Carreau Carreau added this to the 7.1 milestone Oct 3, 2018

@Carreau Carreau referenced this pull request Oct 3, 2018

Closed

HTML vs Iframe #9343

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