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.

Sign up for GitHub

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

don't 'restore_bytes' in from_JSON #4043

Merged
merged 8 commits into from Sep 6, 2013
Merged

don't 'restore_bytes' in from_JSON #4043

merged 8 commits into from Sep 6, 2013

Conversation

minrk
Copy link
Member

@minrk minrk commented Aug 15, 2013

It makes no sense to turn base64-encoded unicode strings into base64-encoded byte strings.
I can't think why we do this, but we should be very careful about testing before merging this fix.

fixes the issue tested in #4036.

@stonebig stonebig mentioned this pull request Aug 15, 2013
Closed
@minrk minrk mentioned this pull request Aug 15, 2013
Closed
@minrk
Copy link
Member Author

minrk commented Aug 15, 2013

Things we need to be careful about (and fully document). With this change, the nbformat clearly asserts that:

The notebook document in-memory contains exclusively JSON-able data. That is, binary data must be base64-encoded and converted to str / unicode before passing it to nbformat.

We were already doing this, but the data types we used did not reflect that, because we were ensuring that we had base64-encoded bytes, which doesn't make any sense to me.

Pinging @ellisonbg for careful review.

@minrk
Copy link
Member Author

minrk commented Aug 15, 2013

Note that this actually didn't fail, it just managed to run tests on 2.6 after we merged @takluyver's drop-2.6 PR.

@minrk
Copy link
Member Author

minrk commented Aug 15, 2013

rebased, let's see if it passes now.

@Carreau
Copy link
Member

Carreau commented Aug 16, 2013

+1

@stonebig stonebig mentioned this pull request Aug 16, 2013
Closed
@ghost ghost assigned ellisonbg Aug 29, 2013
@takluyver
Copy link
Member

@ellisonbg , I think this is on your pile for review. I've assigned it to you.

@ellisonbg
Copy link
Member

OK I will review this...

minrk and others added 7 commits September 3, 2013 15:48
@minrk
don't 'restore_bytes' in from_JSON
e238d5b 
it makes no sense to turn base64-encoded unicode strings into base64-encoded byte strings.  I can't think why we do this, but we should be very careful about testing before merging this fix.

fixes the issue tested in ipython#4036.
@minrk
update test_extract_output
2805f51
@minrk
use cast_unicode in nbformat
aca47fc
@minrk
test that PNG / JPEG output data are unicode
8a8c7e8
@minrk
Test no incorrect 'data:image/png;base64,b' in html files.
f3d217e
@minrk
stonebig's test shouldn't fail anymore
45d15e5
@minrk
note that restore_bytes is never used
f42bb85
@minrk
Copy link
Member Author

minrk commented Sep 3, 2013

I will merge this in the next day or two if I don't hear from anyone.

@minrk
Copy link
Member Author

minrk commented Sep 5, 2013

todo:

  • test b64-bytes
  • test raw bytes

@minrk
test binary data passed to output
88753b5 
data is coerced to unicode (cast with 'replace', so no errors)
@minrk
Copy link
Member Author

minrk commented Sep 5, 2013

both cases are tested - they pass through without errors, because data is coerced to unicode (unrecognized characters are replaced with �). If we want to raise an exception on bad data, I can change that, too.

@ellisonbg
Copy link
Member

This looks great, I am +1 on merging, but I will let you do that.

minrk added a commit that referenced this pull request Sep 6, 2013
@minrk
Merge pull request #4043 from minrk/no-restore-bytes
5a27a13 
don't 'restore_bytes' in from_JSON 

It makes no sense to coerce base64-encoded unicode strings into base64-encoded byte strings.

closes #4036
@minrk minrk merged commit 5a27a13 into ipython:master Sep 6, 2013
minrk added a commit that referenced this pull request Sep 9, 2013
@minrk
Backport PR #4043: don't 'restore_bytes' in from_JSON
3ff9c5c 
It makes no sense to turn base64-encoded unicode strings into base64-encoded byte strings.
I can't think why we do this, but we should be very careful about testing before merging this fix.

fixes the issue tested in #4036.
@minrk minrk deleted the no-restore-bytes branch March 31, 2014 23:36
yarikoptic added a commit to yarikoptic/ipython that referenced this pull request May 2, 2014
@yarikoptic
Merge commit 'rel-1.1.0-3-gb8b89ca' into debian-1X
d96250d 
* commit 'rel-1.1.0-3-gb8b89ca': (66 commits)
  Backport PR ipython#4209: Magic doc fixes
  Backport PR ipython#4204: remove some extraneous print statements from IPython.parallel
  back to dev
  release 1.1.0
  don't upload to GitHub in release script
  1.1 backport stats
  Backport PR ipython#4188: Allow user_ns trait to be None
  Backport PR ipython#4189: always fire LOCAL_IPS.extend(PUBLIC_IPS)
  Backport PR ipython#4174: various issues in markdown and rst templates
  Backport PR ipython#4181: nbconvert: Fix, sphinx template not removing new lines from headers
  Backport PR ipython#4043: don't 'restore_bytes' in from_JSON
  Backport PR ipython#4178: add missing data_javascript
  Backport PR ipython#4136: catch javascript errors in any output
  Backport PR ipython#4163: Fix for incorrect default encoding on Windows.
  Backport PR ipython#4171: add nbconvert config file when creating profiles
  Backport PR ipython#4159: don't split `.cell` and `div.cell` CSS
  Backport PR ipython#4158: generate choices for `--gui` configurable from real mapping
  Backport PR ipython#4143: update example custom.js
  Backport PR ipython#4144: help_end transformer shouldn't pick up ? in multiline string
  Backport PR ipython#4104: Add way to install MathJax to a particular profile
  ...
yarikoptic added a commit to yarikoptic/ipython that referenced this pull request May 2, 2014
@yarikoptic
Merge commit 'rel-1.1.0-7-gf5891e9' into releases
38e04d0 
* commit 'rel-1.1.0-7-gf5891e9': (70 commits)
  Backport PR ipython#4346: getpass() on Windows & Python 2 needs bytes prompt
  Backport PR ipython#4336: use simple replacement rather than string formatting in format_kernel_cmd
  Backport PR ipython#4316: underscore missing on notebook_p4
  Backport PR ipython#4257: fix unicode argv parsing
  Backport PR ipython#4209: Magic doc fixes
  Backport PR ipython#4204: remove some extraneous print statements from IPython.parallel
  back to dev
  release 1.1.0
  don't upload to GitHub in release script
  1.1 backport stats
  Backport PR ipython#4188: Allow user_ns trait to be None
  Backport PR ipython#4189: always fire LOCAL_IPS.extend(PUBLIC_IPS)
  Backport PR ipython#4174: various issues in markdown and rst templates
  Backport PR ipython#4181: nbconvert: Fix, sphinx template not removing new lines from headers
  Backport PR ipython#4043: don't 'restore_bytes' in from_JSON
  Backport PR ipython#4178: add missing data_javascript
  Backport PR ipython#4136: catch javascript errors in any output
  Backport PR ipython#4163: Fix for incorrect default encoding on Windows.
  Backport PR ipython#4171: add nbconvert config file when creating profiles
  Backport PR ipython#4159: don't split `.cell` and `div.cell` CSS
  ...
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
@minrk
Merge pull request ipython#4043 from minrk/no-restore-bytes
dc4bc84 
don't 'restore_bytes' in from_JSON 

It makes no sense to coerce base64-encoded unicode strings into base64-encoded byte strings.

closes ipython#4036
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@ellisonbg ellisonbg

Labels
None yet
Projects
None yet
Milestone
2.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@minrk @Carreau @takluyver @ellisonbg