Repository: executablebooks/mdit-py-plugins

Issue:
Do not allow blank lines within $$
See https://github.com/executablebooks/markdown-it-dollarmath/pull/8

Discussion:
eric-wieser: I think a maintainer (@chrisjsewell?) has to click some sort of approval button in order for CI to run, to verify that I'm not mining cryptocurrency in this PR? 
eric-wieser: @chrisjsewell: this is what I am referring to:

![image](https://user-images.githubusercontent.com/425260/176630989-259dcb8c-b5cf-4050-90ca-84796594d139.png)

codecov[bot]: # [Codecov](https://codecov.io/gh/executablebooks/mdit-py-plugins/pull/46?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks) Report
> Merging [#46](https://codecov.io/gh/executablebooks/mdit-py-plugins/pull/46?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks) (1879f17) into [master](https://codecov.io/gh/executablebooks/mdit-py-plugins/commit/e1e5e94f8ad7947f60f78f85befea3997448e04b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks) (e1e5e94) will **increase** coverage by `0.07%`.
> The diff coverage is `100.00%`.

```diff
@@            Coverage Diff             @@
##           master      #46      +/-   ##
==========================================
+ Coverage   93.19%   93.26%   +0.07%     
==========================================
  Files          25       25              
  Lines        1351     1351              
==========================================
+ Hits         1259     1260       +1     
+ Misses         92       91       -1     
```

| Flag | Coverage Δ | |
|---|---|---|
| pytests | `93.26% <100.00%> (+0.07%)` | :arrow_up: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/executablebooks/mdit-py-plugins/pull/46?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks) | Coverage Δ | |
|---|---|---|
| [mdit\_py\_plugins/dollarmath/index.py](https://codecov.io/gh/executablebooks/mdit-py-plugins/pull/46/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks#diff-bWRpdF9weV9wbHVnaW5zL2RvbGxhcm1hdGgvaW5kZXgucHk=) | `90.58% <100.00%> (+0.58%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/executablebooks/mdit-py-plugins/pull/46?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/executablebooks/mdit-py-plugins/pull/46?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks). Last update [e1e5e94...1879f17](https://codecov.io/gh/executablebooks/mdit-py-plugins/pull/46?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks).

eric-wieser: I assume the CI failure due to a bad interaction between click and black is bogus? Thanks for triggering the CI run.
eric-wieser: Can I do anything to help this to go through?
chrisjsewell: > Can I do anything to help this to go through?

I feel this behaviour needs to be under a options flag, (a) because it is a breaking change and (b) because

> This matches the behavior of LaTeX (and of the stackexchange and github markdown parsers), and prevents runaway parsing if a $$ is opened but not closed.

I would suggest this is not always the desired behaviour.
Firstly because in MyST they are intended to be a shorthand for: https://www.sphinx-doc.org/en/master/usage/restructuredtext/directives.html#directive-math, which can have blank lines (and quite probably there is already people using this feature),
and (b) "runaway parsing" is actually a feature of Markdown not a bug, both fence blocks https://spec.commonmark.org/0.30/#example-126, and (extended) markdown divs https://htmlpreview.github.io/?https://github.com/jgm/djot/blob/master/doc/syntax.html#div are both "runaway" (and there is design reasons for this)
chrisjsewell: I think there is also a potentially critical difference between block level `$$` and inline `$$`;
 inline `$$` already does not allow blank lines 
eric-wieser: Does this mean you would advocate for the change to be reverted on the javascript side?
chrisjsewell: > Does this mean you would advocate for the change to be reverted on the javascript side?

Oh I'm not touching lol. Just let's add it in here as a feature flag, and I think that's a good compromise 
eric-wieser: > I would suggest this is not always the desired behaviour in MyST Firstly because in MyST they are partially intended to be a shorthand for: https://www.sphinx-doc.org/en/master/usage/restructuredtext/directives.html#directive-math, which can have blank lines (and quite probably there is already people using this feature)

The semantics of `.. math::` is to [auto-insert an `align` environment](https://github.com/sphinx-doc/sphinx/blob/ba080286b06cb9e0cadec59a6cf1f96aa11aef5a/sphinx/util/math.py#L35-L55) when multiple lines are provided. This is pretty non-standard for a latex parser, possibly suggesting that the MyST behavior ought to be behind a flag

> and (b) "runaway parsing" is actually a feature of Markdown not a bug, both fence blocks https://spec.commonmark.org/0.30/#example-126, and (extended) markdown div blocks https://htmlpreview.github.io/?https://github.com/jgm/djot/blob/master/doc/syntax.html#div are both "runaway" (and there are design reasons for this)

I agree; but on the other hand, it seems more valuable to match what the consensus seems to be among major implementations, rather than extrapolate from a spec that doesn't actually specify the behavior here!


Can I convince you of a feature flag where the default is the new behavior, or is that going to break too many things?
chrisjsewell: > Can I convince you of a feature flag where the default is the new behavior, or is that going to break too many things?

Yeh just to much possibility for breakage for the outset. Of course once it is in there then it's easy to control, if defaults need to be changed
eric-wieser: Alright, I've added the flag to default to the old behavior.
eric-wieser: Note that the tests won't run until you approve them:

![image](https://user-images.githubusercontent.com/425260/222848921-150182fe-3f31-4743-a529-b9f7da71715c.png)


Patch:
diff --git a/mdit_py_plugins/dollarmath/index.py b/mdit_py_plugins/dollarmath/index.py
index 2663617..e2a4fed 100644
--- a/mdit_py_plugins/dollarmath/index.py
+++ b/mdit_py_plugins/dollarmath/index.py
@@ -13,6 +13,7 @@ def dollarmath_plugin(
     allow_labels: bool = True,
     allow_space: bool = True,
     allow_digits: bool = True,
+    allow_blank_lines: bool = True,
     double_inline: bool = False,
     label_normalizer: Optional[Callable[[str], str]] = None,
     renderer: Optional[Callable[[str, Dict[str, Any]], str]] = None,
@@ -30,6 +31,10 @@ def dollarmath_plugin(
     :param allow_digits: Parse inline math when there is a digit
         before/after the opening/closing ``$``, e.g. ``1$`` or ``$2``.
         This is useful when also using currency.
+    :param allow_blank_lines: Allow blank lines inside ``$$``. Note that blank lines are
+        not allowed in LaTeX, executablebooks/markdown-it-dollarmath, or the Github or
+        StackExchange markdown dialects. Hoever, they have special semantics if used
+        within Sphinx `..math` admonitions, so are allowed for backwards-compatibility.
     :param double_inline: Search for double-dollar math within inline contexts
     :param label_normalizer: Function to normalize the label,
         by default replaces whitespace with `-`
@@ -47,7 +52,9 @@ def dollarmath_plugin(
         math_inline_dollar(allow_space, allow_digits, double_inline),
     )
     md.block.ruler.before(
-        "fence", "math_block", math_block_dollar(allow_labels, label_normalizer)
+        "fence",
+        "math_block",
+        math_block_dollar(allow_labels, label_normalizer, allow_blank_lines),
     )
 
     # TODO the current render rules are really just for testing
@@ -246,6 +253,7 @@ DOLLAR_EQNO_REV = re.compile(r"^\s*\)([^)$\r\n]+?)\(\s*\${2}")
 def math_block_dollar(
     allow_labels: bool = True,
     label_normalizer: Optional[Callable[[str], str]] = None,
+    allow_blank_lines: bool = False,
 ) -> Callable[[StateBlock, int, int, bool], bool]:
     """Generate block dollar rule."""
 
@@ -299,15 +307,14 @@ def math_block_dollar(
                 start = state.bMarks[nextLine] + state.tShift[nextLine]
                 end = state.eMarks[nextLine]
 
-                if end - start < 2:
-                    continue
-
                 lineText = state.src[start:end]
 
                 if lineText.strip().endswith("$$"):
                     haveEndMarker = True
                     end = end - 2 - (len(lineText) - len(lineText.strip()))
                     break
+                if lineText.strip() == "" and not allow_blank_lines:
+                    break  # blank lines are not allowed within $$
 
                 # reverse the line and match
                 if allow_labels:


Test patch:
diff --git a/tests/fixtures/dollar_math.md b/tests/fixtures/dollar_math.md
index a1c2d43..c6554ec 100644
--- a/tests/fixtures/dollar_math.md
+++ b/tests/fixtures/dollar_math.md
@@ -226,6 +226,18 @@ b = 2
 </div>
 .
 
+display equation with blank lines. (valid=False)
+.
+$$
+1+1=2
+
+$$
+.
+<p>$$
+1+1=2</p>
+<p>$$</p>
+.
+
 equation followed by a labelled equation (valid=True)
 .
 $$
diff --git a/tests/test_dollarmath.py b/tests/test_dollarmath.py
index 62f52ef..6ae0345 100644
--- a/tests/test_dollarmath.py
+++ b/tests/test_dollarmath.py
@@ -92,7 +92,11 @@ def test_custom_renderer(data_regression):
 )
 def test_dollarmath_fixtures(line, title, input, expected):
     md = MarkdownIt("commonmark").use(
-        dollarmath_plugin, allow_space=False, allow_digits=False, double_inline=True
+        dollarmath_plugin,
+        allow_space=False,
+        allow_digits=False,
+        double_inline=True,
+        allow_blank_lines=False,
     )
     md.options.xhtmlOut = False
     text = md.render(input)