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.

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

Some fixes in IPython Sphinx directive #3298

Merged
merged 4 commits into from Jun 15, 2013
Merged

Conversation

abakan-zz
Copy link

Made two fixes in plot_directive module:

  • a problem with interpreting pure python code with multiple levels of indentation
  • a problem with keeping track of visited files when restarting ipython line numbers

:meth:`IpythonDirective.setup` method was using a temporary file, such
as :file:`/tmp/seen_docRANDOM`, for storing visited source document
filenames. For a document that is not visited (name not in the temporary
file), it restarts counting line numbers.  Temporary file names are
selected randomly and they were not deleted from the disk upon
completion of a build. Since :meth:`.setup` picks a temp file
arbitrarily to visited status of a file is determined incorrectly.
Using a class variable resolves this problem.

Changed the directive setup function to use a class files for keeping
track of documents.
:meth:`EmbeddedSphinxShell.process_pure_python` could not process
block of code with indentation correctly. For example, the following

.. ipython:: python

   for i in range(10):
       if i < 5:
           continue
       else:
           pass

would result in two IPython lines:

In [19]: for i in range(10):
   ....:      if i < 5:
   ....:          continue
   ....:

In [20]: else:
   ....:          pass
   ....:

This would raise various types of error, indentation/snytax, etc.

To resolve this problem, added a simple check for indentation
in the next line which normally start a new IPython line.
@ellisonbg
Copy link
Member

I don't really know anything about this code, @fperez could you have a look?

@Carreau
Copy link
Member

Carreau commented Jun 1, 2013

Reviving this. I don't have much experience in sphinx. Someone else ?

@ariddell
Copy link

ariddell commented Jun 6, 2013

I'm happy to test this. But first -- Which project has ownership of this extension? Is it matplotlib or ipython? It would be nice for there to be only one place to post issues and submit fixes.

Even without this patch there are overlapping fixes.

This is an important fix if it works. It's annoying to have to make sure that one's code only has at most one level.

@minrk
Copy link
Member

minrk commented Jun 6, 2013

It belongs in IPython (and should move to library code).

ref.#3352

@ariddell
Copy link

ariddell commented Jun 6, 2013

In that case there probably should be two separate fixes -- one for the indentation issue (this one seems to include some fixes for other stuff too) and then another to add the fixes already in the matplotlib version AND move it to a place to it can be imported.

Right now if you have matplotlib installed you can (sensibly) import "matplotlib.sphinxext.ipython_directive" in your conf.py. It seems like it would be useful to be able to do the same with ipython -- i.e., be able to import "IPython.sphinxext.ipython_directive"

@minrk
Copy link
Member

minrk commented Jun 6, 2013

In that case there probably should be two separate fixes

Yes, probably. Just answering your question about where ipython_directive should live.

Right now if you have matplotlib installed you can (sensibly) import "matplotlib.sphinxext.ipython_directive" in your conf.py. It seems like it would be useful to be able to do the same with ipython -- i.e., be able to import "IPython.sphinxext.ipython_directive"

Yes, that's what #3352 describes - the matplotlib ipython_directive will be removed, and IPython should move its version into IPython.sphinxext, so people can import it without having to ship a copy.

@ariddell
Copy link

ariddell commented Jun 7, 2013

I just tested the indentation fix. It works! Thanks @abakan. This has been bugging me for some time.

The other changes appear to duplicate some fixes present in matplotlib's current version. Also -- as this patch contains a number of fixes, could it perhaps also include all the Python 3 fixes as well that are present in matplotlib? Or perhaps that should be a third issue?

@ariddell
Copy link

ariddell commented Jun 7, 2013

Sorry @abakan, I see what you've done with the class variable (vs. the tempfiles). Ignore what I said about duplicating code.

It would be nice if it were Python 3 compatible. Maybe this could be tested by dropping it in as a replacement for matplotlib.sphinxext.ipython_directive and seeing if it builds? (Does anyone care about IPython docs building under Python 3?)

@minrk
Copy link
Member

minrk commented Jun 13, 2013

@ariddell does 2to3 handle the Python 3 fixes? Because if so, there shouldn't need to be anything done (yet), because using this from IPython.sphinxext should work on Python 3 as well. If that's okay, then I think this can be merged as-is.

@ariddell
Copy link

It's definitely working fine for Python 2.7.3 and 3.3.0 (after running 2to3). Just tested again...

Once this is committed I'm willing to work on the renaming patch.

@minrk
Copy link
Member

minrk commented Jun 15, 2013

Thanks, merging.

minrk added a commit that referenced this pull request Jun 15, 2013
Some fixes in IPython Sphinx directive

Made two fixes in plot_directive module:

 - a problem with interpreting pure python code with multiple levels of indentation
 - a problem with keeping track of visited files when restarting ipython line numbers
@minrk minrk merged commit 51078a2 into ipython:master Jun 15, 2013
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Some fixes in IPython Sphinx directive

Made two fixes in plot_directive module:

 - a problem with interpreting pure python code with multiple levels of indentation
 - a problem with keeping track of visited files when restarting ipython line numbers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants