issue2053: improve Image format validation and add html width,height #2178

jerryatmda · 2012-07-20T21:48:49Z

This addresses the first two bullets of #2053
Validates first argument to constrain format to png, jpeg,
Lets user specify width, height, or both and emits appropriate html

I introduced a couple of class constants to minimize the number of times I saw 'jpeg' and 'png'
and document the _ACCEPTABLE_FORMATS.

I added one real jpeg image to the test directory and one zero-length gif file to survive the file not found
error in super()

Thanks, Min, for the coaching, I hope it looks adequate.

Closes #2053

Carreau · 2012-07-20T22:09:33Z

Test results for commit cb206f4 merged into master
Platform: darwin

python2.6: OK
python2.7: OK (libraries not available: oct2py pymongo rpy2 wx wx.aui)

Not available for testing: python3.2

Carreau · 2012-07-20T22:17:51Z

IPython/core/display.py

        self.format = unicode(format).lower()
        self.embed = embed if embed is not None else (url is None)
+
+        print ('%s %s %s' % (self.format, self._ACCEPTABLE_EMBEDDINGS, 'T' if self.embed else 'F'))


Is this a forgotten debug statement ?

Yes, whoops.

On Jul 20, 2012, at 5:17 PM, Bussonnier Matthias wrote:

self.format = unicode(format).lower() self.embed = embed if embed is not None else (url is None)

print ('%s %s %s' % (self.format, self._ACCEPTABLE_EMBEDDINGS, 'T' if self.embed else 'F'))

Is this a forgotten debug statement ?

Reply to this email directly or view it on GitHub:
https://github.com/ipython/ipython/pull/2178/files#r1209612

If you make another commit and push, it will be reflected in this PR, so you can address review like this.

I see that I should have commented in here, not outside! I am trying to clean up the thrash below so that I can present you with a clean, simple changeset. There are two small changes I would like to make other than simply deleting the debug statement, but I would rather not have you suffer with all the tripe below.

Carreau · 2012-07-20T22:32:52Z

This seem to work fine for me.

jerryatmda · 2012-07-21T11:51:34Z

Hold this without further attention. My redundant code had a purpose, I foolishly reasoned without testing and broke my test, so I have some thinking to do.

…eight Image now complains if it receives neither filename, url, nor data (param1) test now exhibits graceful behavior

Carreau · 2012-07-21T14:21:44Z

Thanks for the long time trying to rebase :-)

jerryatmda · 2012-07-21T14:26:56Z

IPython/core/display.py

@@ -464,17 +471,27 @@ def __init__(self, data=None, url=None, filename=None, format=u'png', embed=None
            ext = self._find_ext(filename)
        elif url is not None:
            ext = self._find_ext(url)
+        elif data is None:


This forces the use of at least one argument. I believe this is a good, since otherwise, an errant automated process might manage to invoke Image(None)

jerryatmda · 2012-07-21T23:08:26Z

Thanks, Matthias and Min! It was great fun to work with you both; thanks for all your patience. I'll spend some time gearing up on git and see if I can't find a few more ways to be helpful.

Carreau · 2012-07-22T18:01:34Z

Test results for commit 879b843 merged into master
Platform: darwin

python2.6: OK
python2.7: OK (libraries not available: oct2py pymongo rpy2 wx wx.aui)

Not available for testing: python3.2

takluyver · 2012-07-23T12:59:58Z

Test results for commit 879b843 merged into master
Platform: linux2

python2.7: OK (libraries not available: oct2py pymongo tornado wx wx.aui)
python3.2: Failed, log at https://gist.github.com/3163508 (libraries not available: oct2py pymongo wx wx.aui)

Not available for testing: python2.6

takluyver · 2012-07-23T13:05:26Z

I think those failures are unrelated to these changes. If so, I'll have a pull request ready shortly.

takluyver · 2012-07-23T13:35:01Z

Yep, those failures were something else - I've made PR #2189 for them.

Carreau · 2012-07-23T14:06:15Z

Anybody against merging ?
If no objection in 24h, I'll merge this.

bfroehle · 2012-07-23T15:38:16Z

IPython/core/display.py

+                width_by_height.extend(['width="%d"' % self.width])
+            if self.height:
+                width_by_height.extend(['height="%d"' % self.height])
+            return u'<img src="%s" %s/>' % (self.url, ' '.join(width_by_height))


Do we not have any convenience functions for building an HTML tag out of a dictionary? I guess I would have expected us to have something like:

attr = {} attr['src'] = str(self.url) if self.width: attr['width'] = int(self.width) if self.height: attr['height'] = int(self.height) return render_html_tag('img', **attr)

Anyway, in the future this might be written more cleanly as:

width = height = '' if self.width: width = 'width="%d" ' % self.width if self.height: height = 'height="%d" ' % self.height return u'<img src="%s" %s%s/>' % (self.url, width, height)

Ooh, lovely. I looked at the existing string % url code and simply expanded the same style.
I briefly discussed the code with Min, and he graciously ignored the pain in mine.

On Jul 23, 2012, at 10:38 AM, Bradley M. Froehle wrote:

@@ -484,7 +501,12 @@ def reload(self):

def _repr_html_(self): if not self.embed:

return u'<img src="%s" />' % self.url

width_by_height = []

if self.width:

width_by_height.extend(['width="%d"' % self.width])

if self.height:

width_by_height.extend(['height="%d"' % self.height])

return u'<img src="%s" %s/>' % (self.url, ' '.join(width_by_height))

Do we not have any convenience functions for building an HTML tag out of a dictionary? I guess I would have expected us to have something like:

attr = {} attr['src'] = str(self.url) if self.width: attr['width'] = int(self.width) if self.height: attr['height'] = int(self.height) return render_html_tag('img', **attr)

Anyway, in the future this might be written more cleanly as:

width = height = '' if self.width: width = 'width="%d" ' % self.width if self.height: height = 'height="%d" ' % self.height return u'<img src="%s" %s%s/>' % (self.url, width, height)

Reply to this email directly or view it on GitHub:
https://github.com/ipython/ipython/pull/2178/files#r1216363

Well then it's fine with me... :)

Carreau · 2012-07-31T08:33:00Z

IPython/core/tests/test_display.py

+
+def test_image_size():
+    """Simple test for display.Image(args, width=x,height=y)"""
+    thisurl = 'http://www.google.fr/images/srpr/logo3w.png'


Does debian build machines have access to network during build/test time ?

No, but I don't think this test should actually fetch the image - it's just preparing an HTML <img> tag.

Indeed, I should have seen that.

Carreau · 2012-07-31T08:34:49Z

looks great !

bfroehle · 2012-07-31T19:00:04Z

Test results for commit 879b843 merged into master
Platform: linux2

python2.6: Failed, log at https://gist.github.com/3219507 (libraries not available: matplotlib oct2py pymongo qt rpy2 wx wx.aui)
python2.7: OK
python3.2: OK (libraries not available: oct2py pymongo rpy2 wx wx.aui)

Not available for testing:

Carreau · 2012-07-31T19:03:27Z

@jerryatmda has not forced pushed yet.
we can reopen and merge #2219 maybe ?

takluyver · 2012-07-31T19:45:48Z

If we want to use assert_is_not_none, we'll need to add it to this file:

https://github.com/ipython/ipython/blob/master/IPython/testing/nose_assert_methods.py

jerryatmda · 2012-07-31T19:46:41Z

I would be grateful if you did reopen 2219 and then close this one. 2219 is one clean copy. I won't be able to push --force for a day or two.

Carreau · 2012-08-01T14:23:39Z

I'll do a PR containing #2219 and #2226 cherry pick on top of it so that we can merge it.

Carreau · 2012-08-01T14:33:24Z

Closing in favor of #2231.

jerryatmda · 2012-08-01T15:05:09Z

Merci, Mathieu, I am sorry my newbie-ness and infrequent access to the machine on which my local
git repository exists have been so awkward.

Thanks Bradley, for your efforts in dragging the code into line.

I'll go off and understand git before I inflict any more pain on you all.

Carreau · 2012-08-02T12:48:28Z

Don't worry about not beeing a git master :) I don't think any of us is, we are just used to it.
You'll see that with time you don't think of a main repository as you can synchronize all your repository on all your machines so you can do pretty much anything you want from anywhere.

And the best way to learn is to practice, so don't hesitate to send us more PRs.

Carreau reviewed Jul 20, 2012
View reviewed changes

issue2053: improve Image format validation and introduce html width,h…

879b843

…eight Image now complains if it receives neither filename, url, nor data (param1) test now exhibits graceful behavior

jerryatmda reviewed Jul 21, 2012
View reviewed changes

bfroehle reviewed Jul 23, 2012
View reviewed changes

bfroehle mentioned this pull request Jul 31, 2012

New attempt to close 2053 #2219

Closed

Carreau reviewed Jul 31, 2012
View reviewed changes

bfroehle mentioned this pull request Jul 31, 2012

Python 2.6: Add assert_is_none, assert_is_not_none to nose.tools. #2226

Closed

Carreau mentioned this pull request Aug 1, 2012

Improve Image format validation and add html width,height #2231

Merged

Carreau closed this Aug 1, 2012

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

issue2053: improve Image format validation and add html width,height #2178

issue2053: improve Image format validation and add html width,height #2178

jerryatmda commented Jul 20, 2012

Carreau commented Jul 20, 2012

Carreau Jul 20, 2012

jerryatmda Jul 20, 2012

minrk Jul 21, 2012

jerryatmda Jul 21, 2012

Carreau commented Jul 20, 2012

jerryatmda commented Jul 21, 2012

Carreau commented Jul 21, 2012

jerryatmda Jul 21, 2012

jerryatmda commented Jul 21, 2012

Carreau commented Jul 22, 2012

takluyver commented Jul 23, 2012

takluyver commented Jul 23, 2012

takluyver commented Jul 23, 2012

Carreau commented Jul 23, 2012

bfroehle Jul 23, 2012

jerryatmda Jul 23, 2012

bfroehle Jul 31, 2012

Carreau Jul 31, 2012

takluyver Jul 31, 2012

Carreau Jul 31, 2012

Carreau commented Jul 31, 2012

bfroehle commented Jul 31, 2012

Carreau commented Jul 31, 2012

takluyver commented Jul 31, 2012

jerryatmda commented Jul 31, 2012

Carreau commented Aug 1, 2012

Carreau commented Aug 1, 2012

jerryatmda commented Aug 1, 2012

Carreau commented Aug 2, 2012

issue2053: improve Image format validation and add html width,height #2178

issue2053: improve Image format validation and add html width,height #2178

Conversation

jerryatmda commented Jul 20, 2012

Carreau commented Jul 20, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Carreau commented Jul 20, 2012

jerryatmda commented Jul 21, 2012

Carreau commented Jul 21, 2012

Choose a reason for hiding this comment

jerryatmda commented Jul 21, 2012

Carreau commented Jul 22, 2012

takluyver commented Jul 23, 2012

takluyver commented Jul 23, 2012

takluyver commented Jul 23, 2012

Carreau commented Jul 23, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Carreau commented Jul 31, 2012

bfroehle commented Jul 31, 2012

Carreau commented Jul 31, 2012

takluyver commented Jul 31, 2012

jerryatmda commented Jul 31, 2012

Carreau commented Aug 1, 2012

Carreau commented Aug 1, 2012

jerryatmda commented Aug 1, 2012

Carreau commented Aug 2, 2012