Fix %notebook magic, etc. nbformat unicode tests and fixes#1480
Conversation
avoids unnecessary breakage of backwards-compatible interface when creating new notebook.
There was a problem hiding this comment.
The default encoding is platform dependent (not perhaps the best design, but that's the way things are), so we should explicitly specify encoding='utf-8' for reading & writing JSON files.
There was a problem hiding this comment.
Ah, good call. Should we be using io.open in utils.py3compat, and just use that?
There was a problem hiding this comment.
I think it's simpler to use io.open directly - it makes it obvious that we're not doing any extra magic. Also, it's identical to the Python 3 built-in open(), so there's less to change when we eventually drop py3compat.
There was a problem hiding this comment.
okay, makes sense. Should we just remove the open stuff from py3compat, then?
Seems like we should either:
a) remove open from py3compat
b) on py2, use open = io.open, rather than the custom class we have currently
There was a problem hiding this comment.
Yes, I'd side with removing it. I didn't know about io.open when I wrote py3compat.open (codecs.open is more often mentioned, but doesn't have universal newlines). I'll get onto it in the next couple of days unless you want to do it as part of this.
There was a problem hiding this comment.
Okay, let's do that separately.
There was a problem hiding this comment.
I think this can open Python files, which aren't necessarily UTF-8, although most are. I was preparing code somewhere to sniff the encoding magic comment and open a Python file accordingly, so let's just make a note to hook that up to this when I add that in.
Principally important for distinguishing a sequence of `print n,` messages from a sequence of `print n` messages.
|
This PR now closes the issue reported in #1545 |
|
On quick read this looks good, and tests pass, but I'm too tired to finish the review now; will continue tomorrow. @takluyver, do you have any remaining concerns with this one? If not, I'll give it a pass tomorrow and assuming all looks OK I'll merge it. |
There was a problem hiding this comment.
I think all four of these should probably be unicode literals - i.e. u"u = {u}'héllo'". 8-bit strings with non-ascii characters are a bit of a headache.
There was a problem hiding this comment.
When I make these unicode literals, I get:
AttributeError: 'unicode' object attribute '__doc__' is read-only
Should I leave them as str literals, then?
There was a problem hiding this comment.
Oops, that's a bug in py3compat. It should be basestring rather than str on this line: https://github.com/ipython/ipython/blob/master/IPython/utils/py3compat.py#L35 . You can just make the change in this branch.
|
Besides that comment, I think it's all OK. |
|
Thanks @takluyver; I'll then wait for @minrk to have a chance to fix this before proceeding further. |
There was a problem hiding this comment.
Any particular reason to not follow the naming pattern for test files of having them all be called test_*? I know nose will recognize this as well, but since we already have a pattern for those filenames, I think it would be best to follow it. That makes it easier to know that all files not named test_X.py are simply auxiliary tools for the test suite.
There was a problem hiding this comment.
Yes, because it shouldn't (and doesn't) pick up this one. It's a base class for real tests to inherit from, and won't work without subclassing. It's the same as clienttest in IPython.parallel.
There was a problem hiding this comment.
Ah, OK. I got confused b/c there was a TestCase subclass in there, which I thought was usable by itself. In a similar situation, the pattern I use is to call that class FooTestBase, and not subclass TestCase, so that it's clear that this is an incomplete class, meant to be used as a mixin. Then, the real test class will do class FooTestCase(FooTestBase, TestCase). It's slightly more verbose, but it makes the code more self-documenting, I think.
There was a problem hiding this comment.
That makes good sense, and I will make the change to avoid future similar confusion.
|
OK, I'm good with this going in once @takluyver's minor fix is made. |
|
Okay, now using unicode literals (and @takluyver's tiny fix to py3compat required for that to work), and NBFormatTest is a mixin as recommend by @fperez. Good to go? |
|
OK by me. |
Fix %notebook magic, etc. nbformat unicode tests and fixes. * json.writes always gives unicode, so that `current.writes` can be trusted to give the same interface * setup base TestCase for nbformat tests, to consolidate code, and better test both file formats * add tests for reading/writing to files * allow `name` as kwarg to new_notebook to avoid unnecessary breakage of previous API. * remove fallback to xml, which would hide corrupt notebook files behind a nonsensical 'xml unsupported' message. Closes #1545, #1487.
Fix %notebook magic, etc. nbformat unicode tests and fixes. * json.writes always gives unicode, so that `current.writes` can be trusted to give the same interface * setup base TestCase for nbformat tests, to consolidate code, and better test both file formats * add tests for reading/writing to files * allow `name` as kwarg to new_notebook to avoid unnecessary breakage of previous API. * remove fallback to xml, which would hide corrupt notebook files behind a nonsensical 'xml unsupported' message. Closes ipython#1545, ipython#1487.
3 participants
current.writescan be trusted to give the same interfacenameas kwarg to new_notebook to avoid unnecessary breakage of previous API.