Repository: executablebooks/MyST-Parser
Pull request #919 (2024-04-23 14:24:43)

🔧 More improvements for directive option parsing

codecov-commenter: ## [Codecov](https://app.codecov.io/gh/executablebooks/MyST-Parser/pull/919?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks) Report
Attention: Patch coverage is `94.28571%` with `2 lines` in your changes are missing coverage. Please review.
> Project coverage is 90.13%. Comparing base [(`8614eca`)](https://app.codecov.io/gh/executablebooks/MyST-Parser/commit/8614ecac7233351cc9e11ec625a2d711357b7222?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks) to head [(`8cd5441`)](https://app.codecov.io/gh/executablebooks/MyST-Parser/pull/919?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks).

| [Files](https://app.codecov.io/gh/executablebooks/MyST-Parser/pull/919?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks) | Patch % | Lines |
|---|---|---|
| [myst\_parser/parsers/options.py](https://app.codecov.io/gh/executablebooks/MyST-Parser/pull/919?src=pr&el=tree&filepath=myst_parser%2Fparsers%2Foptions.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks#diff-bXlzdF9wYXJzZXIvcGFyc2Vycy9vcHRpb25zLnB5) | 92.30% | [2 Missing :warning: ](https://app.codecov.io/gh/executablebooks/MyST-Parser/pull/919?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks) |

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master     #919      +/-   ##
==========================================
- Coverage   90.14%   90.13%   -0.02%     
==========================================
  Files          24       24              
  Lines        3400     3415      +15     
==========================================
+ Hits         3065     3078      +13     
- Misses        335      337       +2     
```

| [Flag](https://app.codecov.io/gh/executablebooks/MyST-Parser/pull/919/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks) | Coverage Δ | |
|---|---|---|
| [pytests](https://app.codecov.io/gh/executablebooks/MyST-Parser/pull/919/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks) | `90.13% <94.28%> (-0.02%)` | :arrow_down: |

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.


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/executablebooks/MyST-Parser/pull/919?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks).


Patch:
diff --git a/docs/syntax/roles-and-directives.md b/docs/syntax/roles-and-directives.md
index 8e05b59..a15a896 100644
--- a/docs/syntax/roles-and-directives.md
+++ b/docs/syntax/roles-and-directives.md
@@ -73,18 +73,16 @@ print(f'my {a}nd line')
 ```
 :::
 
-Comments, starting `#`, are also allowed in between options or at the end of values, and are ignored.
 The values can be enclosed in quotes (`"` or `'`) and span multiple lines.
 Newline behaviour can be controlled by starting the value with `|` (preserve newlines) or `>` (collapse newlines):
 
 :::{myst-example}
 ```{code-block} python
-:lineno-start: 10  # this is a comment
-: # this is also a comment
+:lineno-start: 10
 :emphasize-lines: "1, 3"
 :caption: |
-:    This is my
-:    multi-line caption. It is *pretty nifty* ;-)
+:     This is my
+:     multi-line caption. It is *pretty nifty* ;-)
 
 a = 2
 print('my 1st line')
diff --git a/myst_parser/parsers/directives.py b/myst_parser/parsers/directives.py
index 43cde37..5a2a7f4 100644
--- a/myst_parser/parsers/directives.py
+++ b/myst_parser/parsers/directives.py
@@ -188,16 +188,13 @@ def _parse_directive_options(
             yaml_block = content
             content = ""
         yaml_block = dedent(yaml_block)
-    elif content.lstrip().startswith(":"):
-        # TODO deprecate allowing initial whitespace (by lstripping)
-        # or at least make it that all have the same indent
-        # also look at mystjs implementation
+    elif content.startswith(":"):
         content_lines = content.splitlines()
         yaml_lines = []
         while content_lines:
-            if not content_lines[0].lstrip().startswith(":"):
+            if not content_lines[0].startswith(":"):
                 break
-            yaml_lines.append(content_lines.pop(0).lstrip()[1:])
+            yaml_lines.append(content_lines.pop(0)[1:])
         yaml_block = "\n".join(yaml_lines)
         content = "\n".join(content_lines)
 
@@ -227,10 +224,13 @@ def _parse_directive_options(
             )
         return _DirectiveOptions(content, yaml_options, yaml_errors, has_options_block)
 
+    validation_errors: list[ParseWarnings] = []
+
     options: dict[str, str] = {}
     if yaml_block is not None:
         try:
-            options = dict(options_to_items(yaml_block))
+            _options, state = options_to_items(yaml_block)
+            options = dict(_options)
         except TokenizeError as err:
             return _DirectiveOptions(
                 content,
@@ -244,6 +244,14 @@ def _parse_directive_options(
                 ],
                 has_options_block,
             )
+        if state.has_comments:
+            validation_errors.append(
+                ParseWarnings(
+                    "Directive options has # comments, which may not be supported in future versions.",
+                    line,
+                    MystWarnings.DIRECTIVE_OPTION_COMMENTS,
+                )
+            )
 
     if issubclass(directive_class, TestDirective):
         # technically this directive spec only accepts one option ('option')
@@ -258,7 +266,6 @@ def _parse_directive_options(
     options_spec: dict[str, Callable] = directive_class.option_spec
     unknown_options: list[str] = []
     new_options: dict[str, Any] = {}
-    validation_errors: list[ParseWarnings] = []
     value: str | None
     for name, value in options.items():
         try:
diff --git a/myst_parser/parsers/options.py b/myst_parser/parsers/options.py
index 8fe2758..cf4d353 100644
--- a/myst_parser/parsers/options.py
+++ b/myst_parser/parsers/options.py
@@ -163,9 +163,14 @@ class TokenizeError(Exception):
         return "\n".join(lines)
 
 
+@dataclass
+class State:
+    has_comments: bool = False
+
+
 def to_items(
     text: str, line_offset: int = 0, column_offset: int = 0
-) -> Iterable[tuple[str, str]]:
+) -> tuple[list[tuple[str, str]], State]:
     """Parse a directive option block into (key, value) tuples.
 
     :param text: The directive option text.
@@ -174,12 +179,17 @@ def to_items(
 
     :raises: `TokenizeError`
     """
-    for key_token, value_token in to_tokens(text, line_offset, column_offset):
-        yield key_token.value, value_token.value if value_token is not None else ""
+    output = []
+    state = State()
+    for key_token, value_token in _to_tokens(text, state, line_offset, column_offset):
+        output.append(
+            (key_token.value, value_token.value if value_token is not None else "")
+        )
+    return output, state
 
 
-def to_tokens(
-    text: str, line_offset: int = 0, column_offset: int = 0
+def _to_tokens(
+    text: str, state: State, line_offset: int = 0, column_offset: int = 0
 ) -> Iterable[tuple[KeyToken, ValueToken | None]]:
     """Parse a directive option, and yield key/value token pairs.
 
@@ -191,7 +201,7 @@ def to_tokens(
     """
     key_token: KeyToken | None = None
     try:
-        for token in tokenize(text):
+        for token in _tokenize(text, state):
             if isinstance(token, KeyToken):
                 if key_token is not None:
                     yield key_token, None
@@ -207,12 +217,12 @@ def to_tokens(
         raise
 
 
-def tokenize(text: str) -> Iterable[Token]:
+def _tokenize(text: str, state: State) -> Iterable[Token]:
     """Yield tokens from a directive option stream."""
     stream = StreamBuffer(text)
 
     while True:
-        _scan_to_next_token(stream)
+        _scan_to_next_token(stream, state)
 
         if stream.peek() == _CHARS_END:
             break
@@ -227,9 +237,9 @@ def tokenize(text: str) -> Iterable[Token]:
         if ch in ("'", '"'):
             yield _scan_flow_scalar(stream, cast(Literal['"', "'"], ch), is_key=True)
         else:
-            yield _scan_plain_scalar(stream, is_key=True)
+            yield _scan_plain_scalar(stream, state, is_key=True)
 
-        _scan_to_next_token(stream)
+        _scan_to_next_token(stream, state)
 
         # check next char is colon + space
         if stream.peek() != ":":
@@ -240,21 +250,21 @@ def tokenize(text: str) -> Iterable[Token]:
         end_mark = stream.get_position()
         yield ColonToken(start_mark, end_mark)
 
-        _scan_to_next_token(stream)
+        _scan_to_next_token(stream, state)
 
         # now find value
         ch = stream.peek()
         if stream.column == 0:
             pass
         elif ch in ("|", ">"):
-            yield _scan_block_scalar(stream, cast(Literal["|", ">"], ch))
+            yield _scan_block_scalar(stream, cast(Literal["|", ">"], ch), state)
         elif ch in ("'", '"'):
             yield _scan_flow_scalar(stream, cast(Literal['"', "'"], ch), is_key=False)
         else:
-            yield _scan_plain_scalar(stream, is_key=False)
+            yield _scan_plain_scalar(stream, state, is_key=False)
 
 
-def _scan_to_next_token(stream: StreamBuffer) -> None:
+def _scan_to_next_token(stream: StreamBuffer, state: State) -> None:
     """Skip spaces, line breaks and comments.
 
     The byte order mark is also stripped,
@@ -267,6 +277,7 @@ def _scan_to_next_token(stream: StreamBuffer) -> None:
         while stream.peek() == " ":
             stream.forward()
         if stream.peek() == "#":
+            state.has_comments = True
             while stream.peek() not in _CHARS_END_NEWLINE:
                 stream.forward()
         if not _scan_line_break(stream):
@@ -274,7 +285,7 @@ def _scan_to_next_token(stream: StreamBuffer) -> None:
 
 
 def _scan_plain_scalar(
-    stream: StreamBuffer, is_key: bool = False
+    stream: StreamBuffer, state: State, is_key: bool = False
 ) -> KeyToken | ValueToken:
     chunks = []
     start_mark = stream.get_position()
@@ -284,6 +295,7 @@ def _scan_plain_scalar(
     while True:
         length = 0
         if stream.peek() == "#":
+            state.has_comments = True
             break
         while True:
             ch = stream.peek(length)
@@ -302,6 +314,8 @@ def _scan_plain_scalar(
         end_mark = stream.get_position()
         spaces = _scan_plain_spaces(stream, allow_newline=(not is_key))
         if not spaces or stream.peek() == "#" or (stream.column < indent):
+            if stream.peek() == "#":
+                state.has_comments = True
             break
 
     return (
@@ -472,7 +486,9 @@ def _scan_flow_scalar_breaks(stream: StreamBuffer) -> list[str]:
             return chunks
 
 
-def _scan_block_scalar(stream: StreamBuffer, style: Literal["|", ">"]) -> ValueToken:
+def _scan_block_scalar(
+    stream: StreamBuffer, style: Literal["|", ">"], state: State
+) -> ValueToken:
     indent = 0
     folded = style == ">"
     chunks = []
@@ -481,7 +497,7 @@ def _scan_block_scalar(stream: StreamBuffer, style: Literal["|", ">"]) -> ValueT
     # Scan the header.
     stream.forward()
     chomping, increment = _scan_block_scalar_indicators(stream, start_mark)
-    _scan_block_scalar_ignored_line(stream, start_mark)
+    _scan_block_scalar_ignored_line(stream, start_mark, state)
 
     # Determine the indentation level and go to the first non-empty line.
     min_indent = indent + 1
@@ -575,10 +591,13 @@ def _scan_block_scalar_indicators(
     return chomping, increment
 
 
-def _scan_block_scalar_ignored_line(stream: StreamBuffer, start_mark: Position) -> None:
+def _scan_block_scalar_ignored_line(
+    stream: StreamBuffer, start_mark: Position, state: State
+) -> None:
     while stream.peek() == " ":
         stream.forward()
     if stream.peek() == "#":
+        state.has_comments = True
         while stream.peek() not in _CHARS_END_NEWLINE:
             stream.forward()
     ch = stream.peek()
diff --git a/myst_parser/warnings_.py b/myst_parser/warnings_.py
index 18dea03..9dcde95 100644
--- a/myst_parser/warnings_.py
+++ b/myst_parser/warnings_.py
@@ -34,6 +34,8 @@ class MystWarnings(Enum):
     """Issue parsing directive."""
     DIRECTIVE_OPTION = "directive_option"
     """Issue parsing directive options."""
+    DIRECTIVE_OPTION_COMMENTS = "directive_comments"
+    """Directive options has # comments, which may not be supported in future versions."""
     DIRECTIVE_BODY = "directive_body"
     """Issue parsing directive body."""
     UNKNOWN_DIRECTIVE = "directive_unknown"


Test patch:
diff --git a/tests/test_renderers/fixtures/directive_parsing.txt b/tests/test_renderers/fixtures/directive_parsing.txt
index 8f4f601..663d09e 100644
--- a/tests/test_renderers/fixtures/directive_parsing.txt
+++ b/tests/test_renderers/fixtures/directive_parsing.txt
@@ -57,6 +57,25 @@ options:
 warnings: []
 .
 
+note: comment in option
+.
+```{note}
+:class: name  # oops
+a
+```
+.
+arguments: []
+body:
+- a
+content_offset: 1
+options:
+  class:
+  - name
+warnings:
+- 'ParseWarnings(msg=''Directive options has # comments, which may not be supported
+  in future versions.'', lineno=0, type=<MystWarnings.DIRECTIVE_OPTION_COMMENTS: ''directive_comments''>)'
+.
+
 note: content after option with new line
 .
 ```{note}
diff --git a/tests/test_renderers/fixtures/option_parsing.yaml b/tests/test_renderers/fixtures/option_parsing.yaml
index f48b60e..14fe0a5 100644
--- a/tests/test_renderers/fixtures/option_parsing.yaml
+++ b/tests/test_renderers/fixtures/option_parsing.yaml
@@ -7,24 +7,27 @@ plain key/values:
     key4: val4.1
         val4.2
   expected: |-
-    [
-      [
-        "key1",
-        ""
+    {
+      "dict": [
+        [
+          "key1",
+          ""
+        ],
+        [
+          "key2",
+          "val2"
+        ],
+        [
+          "key3",
+          "val3"
+        ],
+        [
+          "key4",
+          "val4.1 val4.2"
+        ]
       ],
-      [
-        "key2",
-        "val2"
-      ],
-      [
-        "key3",
-        "val3"
-      ],
-      [
-        "key4",
-        "val4.1 val4.2"
-      ]
-    ]
+      "comments": false
+    }
 
 plain key/values with comments:
   content: |-
@@ -37,24 +40,27 @@ plain key/values with comments:
     key4: val4.1
         val4.2 # comment
   expected: |-
-    [
-      [
-        "key1",
-        ""
-      ],
-      [
-        "key2",
-        "val2"
+    {
+      "dict": [
+        [
+          "key1",
+          ""
+        ],
+        [
+          "key2",
+          "val2"
+        ],
+        [
+          "key3",
+          "val3"
+        ],
+        [
+          "key4",
+          "val4.1 val4.2"
+        ]
       ],
-      [
-        "key3",
-        "val3"
-      ],
-      [
-        "key4",
-        "val4.1 val4.2"
-      ]
-    ]
+      "comments": true
+    }
 
 quoted key/values:
   content: |-
@@ -65,24 +71,27 @@ quoted key/values:
     3"
     escapes: "\"\e\x07"
   expected: |-
-    [
-      [
-        "key1",
-        "val1"
-      ],
-      [
-        "key2",
-        "val2"
+    {
+      "dict": [
+        [
+          "key1",
+          "val1"
+        ],
+        [
+          "key2",
+          "val2"
+        ],
+        [
+          "key 3",
+          "val 3"
+        ],
+        [
+          "escapes",
+          "\"\u001b\u0007"
+        ]
       ],
-      [
-        "key 3",
-        "val 3"
-      ],
-      [
-        "escapes",
-        "\"\u001b\u0007"
-      ]
-    ]
+      "comments": false
+    }
 
 literal values:
   content: |
@@ -107,24 +116,27 @@ literal values:
 
       val4.3
   expected: |-
-    [
-      [
-        "key1",
-        "val1.1\nval1.2\n\nval1.3\n"
-      ],
-      [
-        "key2",
-        "  val2.1\nval2.2\n\n  val2.3\n"
-      ],
-      [
-        "key3",
-        "val3.1\nval3.2\n\nval3.3"
+    {
+      "dict": [
+        [
+          "key1",
+          "val1.1\nval1.2\n\nval1.3\n"
+        ],
+        [
+          "key2",
+          "  val2.1\nval2.2\n\n  val2.3\n"
+        ],
+        [
+          "key3",
+          "val3.1\nval3.2\n\nval3.3"
+        ],
+        [
+          "key4",
+          "val4.1\nval4.2\n\nval4.3\n"
+        ]
       ],
-      [
-        "key4",
-        "val4.1\nval4.2\n\nval4.3\n"
-      ]
-    ]
+      "comments": false
+    }
 
 folded values:
   content: |
@@ -149,21 +161,24 @@ folded values:
 
       val4.3
   expected: |-
-    [
-      [
-        "key1",
-        "val1.1 val1.2\nval1.3\n"
-      ],
-      [
-        "key2",
-        "  val2.1\nval2.2\n\n  val2.3\n"
-      ],
-      [
-        "key3",
-        "val3.1 val3.2\nval3.3"
+    {
+      "dict": [
+        [
+          "key1",
+          "val1.1 val1.2\nval1.3\n"
+        ],
+        [
+          "key2",
+          "  val2.1\nval2.2\n\n  val2.3\n"
+        ],
+        [
+          "key3",
+          "val3.1 val3.2\nval3.3"
+        ],
+        [
+          "key4",
+          "val4.1 val4.2\nval4.3\n"
+        ]
       ],
-      [
-        "key4",
-        "val4.1 val4.2\nval4.3\n"
-      ]
-    ]
+      "comments": false
+    }
diff --git a/tests/test_renderers/test_parse_directives.py b/tests/test_renderers/test_parse_directives.py
index 0cb6292..e6b2c2b 100644
--- a/tests/test_renderers/test_parse_directives.py
+++ b/tests/test_renderers/test_parse_directives.py
@@ -17,9 +17,14 @@ FIXTURE_PATH = Path(__file__).parent.joinpath("fixtures")
 @pytest.mark.param_file(FIXTURE_PATH / "option_parsing.yaml", "yaml")
 def test_option_parsing(file_params):
     """Test parsing of directive options."""
-    result = list(options_to_items(file_params.content))
+    result, state = options_to_items(file_params.content)
     file_params.assert_expected(
-        json.dumps(result, ensure_ascii=False, indent=2), rstrip_lines=True
+        json.dumps(
+            {"dict": result, "comments": state.has_comments},
+            ensure_ascii=False,
+            indent=2,
+        ),
+        rstrip_lines=True,
     )
 
 
@@ -27,7 +32,7 @@ def test_option_parsing(file_params):
 def test_option_parsing_errors(file_params):
     """Test parsing of directive options."""
     try:
-        list(options_to_items(file_params.content))
+        options_to_items(file_params.content)
     except TokenizeError as err:
         result = str(err)
     else: