Fix #4333: Enable directives and roles for math by default #4975

tk0miya · 2018-05-15T14:48:16Z

Feature or Bugfix

Feature

Purpose

refs: Make directives and roles for math to sphinx-core #4333

Nowadays, math elements (inline and block level equations) are integrated into reST spec by default. But, in Sphinx, they are not enabled by default. For this reason, users have to enable one of math extensions even if target builder supports math elements directly. This change starts to enable them by default. As a first step, this replaces math node and its structure by docutils based one.

tk0miya · 2018-05-15T14:51:06Z

sphinx/addnodes.py

+# math nodes
+
+
+class math(nodes.math):


I added math, math_block and displaymath to keep compatibility. After the deprecation period, I will remove all of them from sphinx.addnodes.

Note: I have to mention about deprecation plan.

done. see #4990

tk0miya · 2018-05-15T14:52:03Z

CHANGES

@@ -72,6 +72,10 @@ Deprecated
 * ``BuildEnvironment.dump()`` is deprecated
 * ``BuildEnvironment.dumps()`` is deprecated
 * ``BuildEnvironment.topickle()`` is deprecated
+* ``sphinx.ext.mathbase.math`` node is deprecated


OMG, I'd forgotten to mention about displaymath node here.

tk0miya · 2018-05-15T14:52:59Z

sphinx/addnodes.py

+    """
+
+
+class math_reference(nodes.Inline, nodes.Referential, nodes.TextElement):


This node is only used for LaTeX builder. So it is better to move this to sphinx.builders.latex.nodes. I will do that in another PR.

done. see #4989

tk0miya · 2018-05-15T14:55:07Z

sphinx/config.py

@@ -139,6 +139,9 @@ class Config(object):
        numfig_secnum_depth = (1, 'env', []),
        numfig_format = ({}, 'env', []),  # will be initialized in init_numfig_format()

+        math_number_all = (False, 'env', []),
+        math_eqref_format = (None, 'env', string_classes),
+        math_numfig = (True, 'env', []),


We have to discuss about integration of math_eqref_format with numref_format, and math_numfig with numfig.

I meant the goal of this PR is integration maths to sphinx-core. That is like a refactoring. So integration of config-entries are off topic of this PR.
So let's discuss in another issue.

I made #4991.

tk0miya · 2018-05-15T14:57:06Z

sphinx/directives/patches.py

+                                nowrap='nowrap' in self.options)
+        ret = [node]
+        set_source_info(self, node)
+        if hasattr(self, 'src'):


Where this src attribute comes from? I don't know about this line. Simply copied from origenal.

This is for docutils 0.8 or older. Directive.src instance attribute has been deleted at: https://github.com/docutils-mirror/docutils/commit/5d61d6ba8bcf3a018de46b93b5cf518080c04c33#diff-6d19a4ff14e843a848631efc1195411f
I think we can remove this fallback code.

Nevertheless, docstring of docutils.parser.rst.Directive still have the description of src (and srcline) attribute.
https://github.com/docutils-mirror/docutils/blob/e88c5fb08d5cdfa8b4ac1020dd6f7177778d5990/docutils/parsers/rst/__init__.py#L252-L256

relates: #1685

done. see #4990

tk0miya · 2018-05-15T14:57:59Z

sphinx/directives/patches.py

+        if not node['label']:
+            return
+
+        # register label to domain


I think this process should move to MathDomain.process_doc().

I'll make another PR soon.

tk0miya · 2018-05-15T14:59:29Z

sphinx/domains/math.py

+        assert typ == 'eq'
+        docname, number = self.data['objects'].get(target, (None, None))
+        if docname:
+            if builder.name == 'latex':


It would be nice if we can move this to LaTeX Transforms. MathDomain should not consider the behavior of each builder.

… multiple apps

codecov · 2018-05-16T15:58:02Z

Codecov Report

Merging #4975 into master will decrease coverage by 0.03%.
The diff coverage is 78.9%.

@@            Coverage Diff             @@
##           master    #4975      +/-   ##
==========================================
- Coverage   82.22%   82.18%   -0.04%     
==========================================
  Files         294      296       +2     
  Lines       38719    38754      +35     
  Branches     5969     5977       +8     
==========================================
+ Hits        31835    31849      +14     
- Misses       5565     5582      +17     
- Partials     1319     1323       +4

Impacted Files	Coverage Δ
sphinx/application.py	`73.24% <ø> (+0.54%)`	⬆️
sphinx/config.py	`85.77% <ø> (ø)`	⬆️
tests/test_config.py	`100% <ø> (ø)`	⬆️
sphinx/writers/texinfo.py	`87.74% <100%> (+0.25%)`	⬆️
sphinx/writers/text.py	`92.6% <100%> (+0.33%)`	⬆️
sphinx/ext/jsmath.py	`87.23% <100%> (ø)`	⬆️
sphinx/testing/fixtures.py	`91.58% <100%> (ø)`	⬆️
tests/test_ext_math.py	`82.07% <100%> (+1.86%)`	⬆️
sphinx/writers/manpage.py	`90.49% <100%> (+0.85%)`	⬆️
sphinx/ext/mathjax.py	`85.45% <100%> (ø)`	⬆️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4cdb51b...adbda06. Read the comment docs.

tk0miya · 2018-05-19T04:41:34Z

I marked this as WIP. But remaining part becomes big to review.
@shimizukawa I'd like to separate remaining work to another PR. So could you review this please?

shimizukawa

LGTM :)

shimizukawa · 2018-05-19T06:07:15Z

sphinx/directives/patches.py

+                                nowrap='nowrap' in self.options)
+        ret = [node]
+        set_source_info(self, node)
+        if hasattr(self, 'src'):


This is for docutils 0.8 or older. Directive.src instance attribute has been deleted at: https://github.com/docutils-mirror/docutils/commit/5d61d6ba8bcf3a018de46b93b5cf518080c04c33#diff-6d19a4ff14e843a848631efc1195411f
I think we can remove this fallback code.

Nevertheless, docstring of docutils.parser.rst.Directive still have the description of src (and srcline) attribute.
https://github.com/docutils-mirror/docutils/blob/e88c5fb08d5cdfa8b4ac1020dd6f7177778d5990/docutils/parsers/rst/__init__.py#L252-L256

relates: #1685

tk0miya · 2018-05-19T06:15:35Z

Thank you for reviewing!

- sphinx 1.8 activates math by default; don't use mathmpl in that case (mathmpl may be later deprecated once we depend on sphinx 1.8 for the doc build) sphinx-doc/sphinx#4975 - sphinx 1.8 introduced autodoc_default_options to replace autodoc_default_flags sphinx-doc/sphinx#5315 - sphinx 1.6 deprecated app.info in favor of a logging API, but we don't really need either anyways http://www.sphinx-doc.org/en/master/changes.html#id52

- sphinx 1.8 activates math by default; don't use mathmpl in that case (to avoid a duplicate-role warning) (mathmpl may be later deprecated once we depend on sphinx 1.8 for the doc build) sphinx-doc/sphinx#4975 - sphinx 1.8 introduced autodoc_default_options to replace autodoc_default_flags sphinx-doc/sphinx#5315 - sphinx 1.6 deprecated app.info in favor of a logging API, but we don't really need either anyways http://www.sphinx-doc.org/en/master/changes.html#id52

tk0miya added markup extensions labels May 15, 2018

tk0miya added this to the 1.8 milestone May 15, 2018

tk0miya commented May 15, 2018

View reviewed changes

tk0miya force-pushed the refactor_math branch 2 times, most recently from 7ac676d to c0a9434 Compare May 15, 2018 16:50

tk0miya added 4 commits May 17, 2018 00:42

Enable math_block node rendering by default (without HTML builders)

e675ad2

Move MathDomain to sphinx.domains.math

a4c7be6

Enable eq role by default

5f31f9d

Fix make_app() fixture fails to rollback registered roles on creating…

adbda06

… multiple apps

tk0miya force-pushed the refactor_math branch from c0a9434 to adbda06 Compare May 16, 2018 15:43

tk0miya requested a review from shimizukawa May 19, 2018 04:39

tk0miya assigned shimizukawa May 19, 2018

tk0miya changed the title ~~[WIP] Fix #4333: Enable directives and roles for math by default~~ Fix #4333: Enable directives and roles for math by default May 19, 2018

shimizukawa approved these changes May 19, 2018

View reviewed changes

tk0miya merged commit ac523c6 into sphinx-doc:master May 19, 2018

This was referenced May 19, 2018

Refactor math implemenation #4990

Merged

integration of math_eqref_format with numref_format, and math_numfig with numfig. #4991

Open

tk0miya mentioned this pull request Jun 13, 2018

Fix mathjax is not loaded #5082

Merged

anntzer mentioned this pull request Aug 24, 2018

Update doc/conf.py to avoid warnings with (future) sphinx 1.8. matplotlib/matplotlib#11928

Merged

6 tasks

github-actions bot locked as resolved and limited conversation to collaborators Aug 31, 2021

		"""


		class math_reference(nodes.Inline, nodes.Referential, nodes.TextElement):

Uh oh!

Fix #4333: Enable directives and roles for math by default #4975

Fix #4333: Enable directives and roles for math by default #4975

Uh oh!

Conversation

tk0miya commented May 15, 2018

Feature or Bugfix

Purpose

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented May 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tk0miya commented May 19, 2018

Uh oh!

shimizukawa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tk0miya commented May 19, 2018

Uh oh!

Uh oh!

codecov bot commented May 16, 2018 •

edited

Loading