Opened 4 years ago

Closed 4 years ago

#27528 closed enhancement (fixed)

Remove the highlighting patch for Sphinx

Reported by: jhpalmieri Owned by:
Priority: major Milestone: sage-8.8
Component: packages: standard Keywords:
Cc: gh-timokau Merged in:
Authors: John Palmieri, Jeroen Demeyer Reviewers: Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 83436ea (Commits, GitHub, GitLab) Commit: 83436ea82a17406bbc3fdd9e8c386208abc93727
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

The goal of this ticket is to clean up highlighting in the documentation. There are several issues. Right now, we are patching pygments and Sphinx so that they know about the "sage:" prompt. Also, we are telling Sphinx to highlight all code blocks as if they were Python console blocks. There are various other types of code blocks in the Sage docs: Python, Cython, reST, LaTeX, etc., and right now many are not highlighted at all: Sphinx/pygments tries to parse them as the Python console and fails, so does nothing. Even some of the Sage console blocks aren't highlighted correctly, because pygments refuses to parse input like sage: a?: valid IPython but not valid Python.

To improve the situation: Sage's console is based more on IPython than on Python, and IPython comes with a lexer to use when building its own documentation. Furthermore, that lexer explicitly allows for customization of the input prompt. So we can get rid of the prompt patches and use IPython's lexer, and it succeeds at highlighting blocks containing sage: a? and other IPython-type text. The only disadvantage to this is that IPython's lexer doesn't convert <BLANKLINE> in doctest output to a blank line, so we add a Sphinx transform to handle this.

We also mark some non-Python blocks as what they are, to enable their highlighting.

Change History (39)

comment:1 Changed 4 years ago by jhpalmieri

We want Sage's doctest blocks (and most other literal blocks) to be typeset as Python console code. Setting the default highlighting language to pycon does this, and it means that we can then not use the highlighting patch for Sphinx.

Currently in Sage, there are some blocks which are not highlighted correctly. See this page: in the first block (containing CombinatorialFreeModule?), the prompt "sage:" is not highlighted. With the proposed change, that block will still not be highlighted the way we want, and in fact it will produce a warning:

WARNING: Could not lex literal_block as "pycon". Highlighting skipped.

We have set up the docbuilding process so that warnings stop it, so we also need to silence this warning.

There are no doubt some blocks in the Sage code which should not be highlighted using pycon, but as things stand in current Sage, Sphinx already tries to highlight them using the default lexer, which basically means python3 with a graceful failure if it can't parse it as Python 3: see the Sphinx docs. Once we set the highlighting language to something other than default, we lose the graceful failure, and so we have to silence the warnings.

In my spot-checking, the current non-Python console blocks fail their highlighting and so have no highlighting at all, and that behavior continues with this branch. At some point we could try to modify those blocks: something like

Here is a block::

.. highlight:: rest

    stuff

But that can be on another ticket.

comment:2 Changed 4 years ago by jhpalmieri

A few blocks in Sage look like

Traceback:
..   <-- should be exactly three dots
more stuff

Having 2 or 4 dots means that Sphinx can't handle the highlighting: you can see this here, where there are 4 dots instead of 3.

Along the same lines, having extra spaces at the end of NotImplementedError causes highlighting to fail here.

Last edited 4 years ago by jhpalmieri (previous) (diff)

comment:3 Changed 4 years ago by jhpalmieri

Branch: u/jhpalmieri/sphinx-highlight

comment:4 Changed 4 years ago by jhpalmieri

Commit: 1debde6cf246bd2ed7a5f90aa58c877c6c61586f
Status: newneeds_review

New commits:

1debde6trac 27528: remove highlighting.patch for Sphinx

comment:5 Changed 4 years ago by jhpalmieri

By the way, I've tried this with #26451 (Sphinx 1.8.4), #25680 (Python 3.7.2), and both together. No troubles.

comment:6 Changed 4 years ago by gh-timokau

Cc: gh-timokau added

comment:7 Changed 4 years ago by jhpalmieri

Status: needs_reviewneeds_work

comment:8 Changed 4 years ago by git

Commit: 1debde6cf246bd2ed7a5f90aa58c877c6c61586f687b3385e80af4efd6ee8db79200c518a731af1d

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

48b7acdtrac 27528: remove some patches for Sphinx and pygments.
b23b8cctrac 27528: use a custom Sphinx transform to handle <BLANKLINE>
687b338trac 27528: clean up some code-blocks in the reST documentation.

comment:9 Changed 4 years ago by jhpalmieri

Description: modified (diff)
Status: needs_workneeds_review

comment:10 Changed 4 years ago by jhpalmieri

Here is a new approach: I just discovered the IPython lexer, and we should definitely use it rather than Python lexer.

comment:11 Changed 4 years ago by jhpalmieri

A few things to check:

  • See http://doc.sagemath.org/html/en/constructions/interface_issues.html for some code blocks for other languages (LaTeX, BibTeX, shell session). These are not currently highlighted, but they will be with this branch. Same goes for various shell session blocks scattered through the developer's guide.
  • The first block in the tour-help section of the tutorial is not currently highlighted, but it will be with this branch.
  • double check that <BLANKLINE> does not appear in the reference manual. One spot to check: from the main reference manual page, click "Symbolic Calculus" and then "Riemann Mapping". The method "complex_to_spiderweb" (toward the end) has some <BLANKLINE>s in the doctest output, and they should be rendered as blank lines in the reference manual.
Last edited 4 years ago by jhpalmieri (previous) (diff)

comment:12 Changed 4 years ago by jhpalmieri

To do on another ticket: go through more of the reST docs and perhaps the reference manual, marking the non-sage-doctest blocks using .. CODE-BLOCK:: language. I'll open up a followup ticket eventually.

comment:13 Changed 4 years ago by dimpase

Reviewers: Dima Pasechnik
Status: needs_reviewpositive_review

works for me nicely. I looked at few random places in various docs, all good.

comment:14 Changed 4 years ago by jhpalmieri

Thank you for the review.

See #27549 for a followup: more code-block cleanup.

comment:15 Changed 4 years ago by jdemeyer

Status: positive_reviewneeds_work

Remember to change the package version if you change patches.

comment:16 Changed 4 years ago by dimpase

Status: needs_workpositive_review

How about we just mark this ticket to be merged together with #26451 - which does a version bump?

comment:17 Changed 4 years ago by jdemeyer

Status: positive_reviewneeds_work

comment:18 in reply to:  16 Changed 4 years ago by jdemeyer

Replying to dimpase:

How about we just mark this ticket to be merged together with #26451 - which does a version bump?

If it can be merged separately, I see no reason to bundle them.

comment:19 Changed 4 years ago by dimpase

Status: needs_workpositive_review

What's the point of doing this? To do more work than needed? Let's just merge them together.

comment:20 Changed 4 years ago by jdemeyer

Status: positive_reviewneeds_work

comment:21 Changed 4 years ago by jdemeyer

Branch: u/jhpalmieri/sphinx-highlightu/jdemeyer/sphinx-highlight

comment:22 Changed 4 years ago by jdemeyer

Commit: 687b3385e80af4efd6ee8db79200c518a731af1d278fa3b3fca010621d41df9d63e0b11801875adc
Status: needs_workneeds_review

New commits:

278fa3bBump package versions

comment:23 Changed 4 years ago by jdemeyer

Sometimes it takes less effort to do the right thing rather than arguing about it...

comment:24 Changed 4 years ago by dimpase

Status: needs_reviewpositive_review

OK

comment:25 Changed 4 years ago by jdemeyer

Status: positive_reviewneeds_work

Looking at this diff, it seems that indentation is not correct in a few places, for example

+  that the user should be aware of.
+
+   .. CODE-BLOCK:: rest

comment:26 Changed 4 years ago by jdemeyer

I also have doubts about

There are 2 tests in doc/en/developer/coding_in_python.rst that are not being run

comment:27 Changed 4 years ago by jdemeyer

And shouldn't the in2_regex for IPythonConsoleLexer be customized too to match ....: ?

comment:28 Changed 4 years ago by git

Commit: 278fa3b3fca010621d41df9d63e0b11801875adca323c0965f3020045900cdb0fcb7702d4ec56f88

Branch pushed to git repo; I updated commit sha1. New commits:

a323c09Fix a few CODE-BLOCK indentations

comment:29 Changed 4 years ago by git

Commit: a323c0965f3020045900cdb0fcb7702d4ec56f884cabfa73badacb19731e95d959a1cf4bedcf531c

Branch pushed to git repo; I updated commit sha1. New commits:

4cabfa7Fix _test_enough_doctests()

comment:30 Changed 4 years ago by git

Commit: 4cabfa73badacb19731e95d959a1cf4bedcf531c83436ea82a17406bbc3fdd9e8c386208abc93727

Branch pushed to git repo; I updated commit sha1. New commits:

83436eaFix usage of IPythonConsoleLexer

comment:31 Changed 4 years ago by jdemeyer

Authors: John PalmieriJohn Palmieri, Jeroen Demeyer
Description: modified (diff)
Status: needs_workneeds_review

comment:32 Changed 4 years ago by jdemeyer

I didn't understand the rationale for making a difference depending on the Python version running the docbuilder: the sources are the same on Python 2 and Python 3, so the documentation should also be the same. So I'm using the default version (which is currently Python 2) always.

comment:33 Changed 4 years ago by jdemeyer

Please review the last 3 commits.

comment:34 in reply to:  28 ; Changed 4 years ago by dimpase

Replying to git:

Branch pushed to git repo; I updated commit sha1. New commits:

a323c09Fix a few CODE-BLOCK indentations

I thought wrong indentations result in sphinx errors...

comment:35 Changed 4 years ago by jhpalmieri

My one objection is that the version for the pygments package should not say .p0 since there are no patches any more. Is there anything to be done about this? (Aside from waiting for the next release, that is.)

Last edited 4 years ago by jhpalmieri (previous) (diff)

comment:36 in reply to:  34 ; Changed 4 years ago by jhpalmieri

Replying to dimpase:

Replying to git:

Branch pushed to git repo; I updated commit sha1. New commits:

a323c09Fix a few CODE-BLOCK indentations

I thought wrong indentations result in sphinx errors...

Sometimes it just results in Sphinx silently ignoring the directive.

comment:37 in reply to:  36 Changed 4 years ago by jhpalmieri

Replying to jhpalmieri:

Replying to dimpase:

Replying to git:

Branch pushed to git repo; I updated commit sha1. New commits:

a323c09Fix a few CODE-BLOCK indentations

I thought wrong indentations result in sphinx errors...

Sometimes it just results in Sphinx silently ignoring the directive.

Actually this case looked like

text

 .. CODE-BLOCK:: rest

    more text

and Sphinx interpreted the extra space in front of .. CODE-BLOCK... just as indentation, so the code block was highlighted correctly, just indented more than it should have been.

comment:38 Changed 4 years ago by jhpalmieri

Status: needs_reviewpositive_review

Jeroen, thanks for the fixes. There is probably nothing to be done about the pygments version number.

comment:39 Changed 4 years ago by vbraun

Branch: u/jdemeyer/sphinx-highlight83436ea82a17406bbc3fdd9e8c386208abc93727
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.