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

refactored py to ipynb #331

Merged
merged 5 commits into from Dec 22, 2017

Conversation

Projects
None yet
4 participants
@nicain
Contributor

nicain commented Dec 19, 2017

Resolves #322

All tests passing in py27, py35, and py36

testing updates and bug fix done in two separate commits; if you don't want tox additions, I can just cherry-pick the bug fix commit (bd211e5) and reissue the PR

@nicain

This comment has been minimized.

Contributor

nicain commented Dec 20, 2017

Also all tests passing locally on my dev environment for py27 py35 and py36 via tox

Removed controversial changes.
with some additional minor tweaks.
@lesteve

This comment has been minimized.

Contributor

lesteve commented Dec 20, 2017

Thanks a lot for your PR! I remove the controversial changes and pushed a commit with additional minor tweaks.

Can you add a test in tests/test_utils.py, testing both the normal condition and the exception + exception message? Can you also add an entry in CHANGES.rst?

I haven't jumped on the tox bandwagon yet although I have heard it is quite good. Minimal googling seems to indicate that conda and tox do not play nice together, oh well this will have to wait then ...

My 2 cents:

  • short and focussed PRs tend to be merged more quickly. Avoid doing two unrelated things in a single PR.
  • controversial changes even if minor tend to make the review less pleasant for everyone involved (bikeshedding + arguing).
"""Replace .py extension in filename by .ipynb"""
fname_prefix, extension = fname.rsplit('.', 1)
if extension != 'py':
raise RuntimeError(

This comment has been minimized.

@lesteve

lesteve Dec 20, 2017

Contributor

Thinking about it maybe ValueError is better here

@nicain

This comment has been minimized.

Contributor

nicain commented Dec 20, 2017

@lesteve

Can you add a test in tests/test_utils.py, testing both the normal condition and the exception + exception message? Can you also add an entry in CHANGES.rst?

Done.

I haven't jumped on the tox bandwagon yet although I have heard it is quite good. Minimal googling seems to indicate that conda and tox do not play nice together, oh well this will have to wait then ...

I just learned to use tox myself, and I love it. I use conda as well, but I can across a handy trick to use tox in a conda development env: create bare conda envs for the python versions you need tox to run against, and add them as symlinks to your path (for example ln -s /foo/envs/toxpy35/bin/python ./python 3.5, executed from ~/local/bin).

Ill stash the tox changes in a branch on my fork (https://github.com/nicain/sphinx-gallery/tree/enh/tox), in case you ever want to circle back to them.

with pytest.raises(ValueError) as expected_exception:
utils.replace_py_ipynb(file_name+'.txt')
assert 'Unrecognized file extension' in str(expected_exception.value)

This comment has been minimized.

@choldgraf

choldgraf Dec 21, 2017

Contributor

could you add a final line at the end?

This comment has been minimized.

@nicain

nicain Dec 21, 2017

Contributor

Done.

def replace_py_ipynb(fname):
"""Replace .py extension in filename by .ipynb"""
fname_prefix, extension = fname.rsplit('.', 1)

This comment has been minimized.

@choldgraf

This comment has been minimized.

@choldgraf

choldgraf Dec 21, 2017

Contributor

(it seems like the functionality would be the same though...so I don't think it's blocking this PR)

This comment has been minimized.

@nicain

nicain Dec 21, 2017

Contributor

Done.

for file_name in ['some/file/name', '/corner.pycase']:
assert utils.replace_py_ipynb(file_name+'.py') == file_name+'.ipynb'
def test_replace_py_ipynb_improper_input():

This comment has been minimized.

@choldgraf

choldgraf Dec 21, 2017

Contributor

I'd merge these into a single test_replace_py_ipynb() function

This comment has been minimized.

@nicain

nicain Dec 21, 2017

Contributor

Done.

@choldgraf

This comment has been minimized.

Contributor

choldgraf commented Dec 21, 2017

Just a couple of quick thoughts from me - nice catch and thanks for the PR @nicain ! :-)

@nicain nicain changed the title from refactored py to ipynb, and implemeted tox testing to refactored py to ipynb Dec 21, 2017

@nicain

This comment has been minimized.

Contributor

nicain commented Dec 21, 2017

Updated and ready for re-review.

@choldgraf

This comment has been minimized.

Contributor

choldgraf commented Dec 21, 2017

Once tests are passing I'm +1 on this. Will hold off on merging until @lesteve can confirm he's happy too!

Thanks @nicain !

@@ -36,3 +37,16 @@ def __init__(self): # noqa: D102
def __del__(self): # noqa: D105
rmtree(self._path, ignore_errors=True)
def replace_py_ipynb(fname):

This comment has been minimized.

@Titan-C

Titan-C Dec 22, 2017

Member

Isn't it just simpler to have a regex substitution

re.sub('\.py$', '.ipynb', fname)

We have already filtered for python files when collecting files for examples. No need for the value error check.

This comment has been minimized.

@nicain

nicain Dec 22, 2017

Contributor

I like the regex, implemented in commit below. But a little extra early-checking seems safe. If you want me to rip it out, I can do that too.

@lesteve lesteve force-pushed the nicain:322 branch from be95e51 to 04e2a69 Dec 22, 2017

@lesteve

This comment has been minimized.

Contributor

lesteve commented Dec 22, 2017

LGTM, thanks a lot @nicain!

@lesteve lesteve merged commit 5d0588f into sphinx-gallery:master Dec 22, 2017

2 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
def test_replace_py_ipynb():
# Test behavior of function with expected input:
for file_name in ['some/file/name', '/corner.pycase']:

This comment has been minimized.

@lesteve

lesteve Dec 22, 2017

Contributor

Very nice that you added a test for the corner case you bumped into by the way!

@choldgraf

This comment has been minimized.

Contributor

choldgraf commented Dec 22, 2017

woo! way to go @nicain ! 🎉

@nicain

This comment has been minimized.

Contributor

nicain commented Jan 6, 2018

@lesteve Any idea when the next patch revision number bump will occur, and be released to pypi? I have a project that would like SG to pip-install as a dependency, but need this fix for correct behavior. I Just need a rough timeline so that I can plan accordingly :)

@lesteve

This comment has been minimized.

Contributor

lesteve commented Jan 8, 2018

Not really, you can install sphinx-gallery from master in the mean time if you are willing to take the (small) risk:

pip install git+https://github.com/sphinx-gallery/sphinx-gallery
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment