Add Python 3 support #15

Merged
merged 6 commits into from Apr 15, 2015

Conversation

Projects
None yet
2 participants
@jni
Contributor

jni commented Apr 13, 2015

This PR brings nearly full compatibility for Python 3, while maintaining Python 2 compatibility.

The sole exception is the R-markdown test, which for me is failing on master as well. The failures on master and on this PR match.

jni added some commits Apr 13, 2015

Wrap pandoc communicate call with str for Py3
Communicating with subprocess was apparently using the wrong encoding
for Python 3 strings during setup.py.
@aaren

This comment has been minimized.

Show comment
Hide comment
@aaren

aaren Apr 13, 2015

Owner

Thanks for doing this :)

All tests are passing for me on master though. The R test does fail for me with this PR.

The knitr calling has always been a bit fragile so it might be something to do with that.

Owner

aaren commented Apr 13, 2015

Thanks for doing this :)

All tests are passing for me on master though. The R test does fail for me with this PR.

The knitr calling has always been a bit fragile so it might be something to do with that.

@jni

This comment has been minimized.

Show comment
Hide comment
@jni

jni Apr 13, 2015

Contributor

Done! Let me know if this works! This still shows the same failure for me, but might be something weird with my R install. (It's a bit neglected these days!)

Contributor

jni commented Apr 13, 2015

Done! Let me know if this works! This still shows the same failure for me, but might be something weird with my R install. (It's a bit neglected these days!)

@aaren

This comment has been minimized.

Show comment
Hide comment
@aaren

aaren Apr 14, 2015

Owner

Thanks. Python 2 and 3 both passing for me now.

The default for the temp files is 'w+b', which is the cause of the problem on
python 3.

Remaining things to do:

  • update 442bbb as above (open tmp file as 'w+' and remove the 'rb' for
    the template file)
  • squash all of the file IO commits together (the last two and 442bbb) -
    together I think they should amount to just setting mode='w+' for each
    of the temp files.
  • add six to the install_requires in setup.py

Re your failing R test: Does Knitr work for you from the command line? i.e. if
you run

notedown r-examples/r-example.Rmd --knit --to markdown

do you get sensible looking markdown? And if you run

notedown r-examples/r-example.Rmd --knit --rmagic

is the result the same as r-examples/r-example.ipynb? (less the outputs,
which you can generate with --run)

Owner

aaren commented Apr 14, 2015

Thanks. Python 2 and 3 both passing for me now.

The default for the temp files is 'w+b', which is the cause of the problem on
python 3.

Remaining things to do:

  • update 442bbb as above (open tmp file as 'w+' and remove the 'rb' for
    the template file)
  • squash all of the file IO commits together (the last two and 442bbb) -
    together I think they should amount to just setting mode='w+' for each
    of the temp files.
  • add six to the install_requires in setup.py

Re your failing R test: Does Knitr work for you from the command line? i.e. if
you run

notedown r-examples/r-example.Rmd --knit --to markdown

do you get sensible looking markdown? And if you run

notedown r-examples/r-example.Rmd --knit --rmagic

is the result the same as r-examples/r-example.ipynb? (less the outputs,
which you can generate with --run)

@jni

This comment has been minimized.

Show comment
Hide comment
@jni

jni Apr 14, 2015

Contributor

Hey @aaren, what do you mean "442bbb"? From context it sounds like you're talking about 37e4b25? There is no commit 442bbb in this history...

Contributor

jni commented Apr 14, 2015

Hey @aaren, what do you mean "442bbb"? From context it sounds like you're talking about 37e4b25? There is no commit 442bbb in this history...

jni added some commits Apr 13, 2015

Pass empty dict instead of None to IPython exec
Although None worked for earlier versions of IPython, it fails on 3.1.
I checked whether this was a regression in IPython or just the result
of poor specification, and it was the latter.

1. Although the `preprocess` method annoyingly has no docstring, the
   `resources` argument is passed directly on to
   `IPython.nbconvert.preprocessors.base.Preprocessor.preprocess`,
   which does have a docstring.
2. This docstring specifies that `resources` should be a dictionary.
3. This commit appears to have broken the ability to pass `None` as
   `resources`:
   ipython/ipython@af18c6c
Set tempfile mode to 'w+'
Python 3 is unhappy copying text mode files into binary mode
tempfile. This commit standardises all files to be open in text
mode.
@jni

This comment has been minimized.

Show comment
Hide comment
@jni

jni Apr 14, 2015

Contributor

Well, I'm assuming you meant 37e4b25, which I have now reverted and squashed in with all the knitr changes, as well as added six to the requirements, so we should be all good!

As to Knitr, no, not working. Here's my output to notedown r-examples/r-example.Rmd --knit --rmagic:

{
 "cells": [
  {
   "cell_type": "code",
   "execution_count": null,
   "metadata": {},
   "outputs": [],
   "source": [
    "%load_ext rpy2.ipython"
   ]
  }
 ],
 "metadata": {},
 "nbformat": 4,
 "nbformat_minor": 0
}

The first command, notedown r-examples/r-example.Rmd --knit --to markdown, produces no output at all.

Contributor

jni commented Apr 14, 2015

Well, I'm assuming you meant 37e4b25, which I have now reverted and squashed in with all the knitr changes, as well as added six to the requirements, so we should be all good!

As to Knitr, no, not working. Here's my output to notedown r-examples/r-example.Rmd --knit --rmagic:

{
 "cells": [
  {
   "cell_type": "code",
   "execution_count": null,
   "metadata": {},
   "outputs": [],
   "source": [
    "%load_ext rpy2.ipython"
   ]
  }
 ],
 "metadata": {},
 "nbformat": 4,
 "nbformat_minor": 0
}

The first command, notedown r-examples/r-example.Rmd --knit --to markdown, produces no output at all.

@hdashnow hdashnow referenced this pull request in elegant-scipy/elegant-scipy Apr 14, 2015

Closed

Writing workflow - formatting #1

@aaren aaren merged commit 684bf37 into aaren:master Apr 15, 2015

aaren added a commit that referenced this pull request Apr 15, 2015

@aaren

This comment has been minimized.

Show comment
Hide comment
@aaren

aaren Apr 15, 2015

Owner

Merged. Thanks again for doing this.

Do you definitely have knitr installed? i.e. you don't get and error if you
start the R interpreter and then do require(knitr)?

Owner

aaren commented Apr 15, 2015

Merged. Thanks again for doing this.

Do you definitely have knitr installed? i.e. you don't get and error if you
start the R interpreter and then do require(knitr)?

@jni

This comment has been minimized.

Show comment
Hide comment
@jni

jni Apr 15, 2015

Contributor

No worries! My workflow now depends on this and I really want to move to Python 3 full time so it's a very self-interested PR! =) Nonetheless, happy to help!

btw, do you think you might cut a release with this so I don't have to install from github? (not that it's a huge deal, just slightly less elegant.)

Finally, re: knitr, indeed I don't have it installed, so that explains a few things. =P It might be worth raising an error when knitr can't be found, rather than failing silently. But I'll leave that to you. I don't need knitr right now so I'm not worried about this!

Contributor

jni commented Apr 15, 2015

No worries! My workflow now depends on this and I really want to move to Python 3 full time so it's a very self-interested PR! =) Nonetheless, happy to help!

btw, do you think you might cut a release with this so I don't have to install from github? (not that it's a huge deal, just slightly less elegant.)

Finally, re: knitr, indeed I don't have it installed, so that explains a few things. =P It might be worth raising an error when knitr can't be found, rather than failing silently. But I'll leave that to you. I don't need knitr right now so I'm not worried about this!

@aaren

This comment has been minimized.

Show comment
Hide comment
@aaren

aaren Apr 15, 2015

Owner

Sorry, meant to do that already. I've just released notedown 1.4.0.

Yes, raising an error would be preferable. Have opened #18.

On Tue 14 Apr, Juan Nunez-Iglesias wrote:

No worries! My workflow now depends on this and I really want to move to Python 3 full time so it's a very self-interested PR! =) Nonetheless, happy to help!

btw, do you think you might cut a release with this so I don't have to install from github? (not that it's a huge deal, just slightly less elegant.)

Finally, re: knitr, indeed I don't have it installed, so that explains a few things. =P It might be worth raising an error when knitr can't be found, rather than failing silently. But I'll leave that to you. I don't need knitr right now so I'm not worried about this!


Reply to this email directly or view it on GitHub:
#15 (comment)

Owner

aaren commented Apr 15, 2015

Sorry, meant to do that already. I've just released notedown 1.4.0.

Yes, raising an error would be preferable. Have opened #18.

On Tue 14 Apr, Juan Nunez-Iglesias wrote:

No worries! My workflow now depends on this and I really want to move to Python 3 full time so it's a very self-interested PR! =) Nonetheless, happy to help!

btw, do you think you might cut a release with this so I don't have to install from github? (not that it's a huge deal, just slightly less elegant.)

Finally, re: knitr, indeed I don't have it installed, so that explains a few things. =P It might be worth raising an error when knitr can't be found, rather than failing silently. But I'll leave that to you. I don't need knitr right now so I'm not worried about this!


Reply to this email directly or view it on GitHub:
#15 (comment)

@jni jni deleted the jni:py3 branch Apr 15, 2015

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