Conversation
…into feat/support-math
| // `math` option validation | ||
| const mathOption = languageOptions?.math; | ||
|
|
||
| if (mathOption !== undefined && typeof mathOption !== "boolean") { |
There was a problem hiding this comment.
Question:
Are the undefined checks (e.g. mathOption !== undefined, frontmatterOption !== undefined) and the optional chaining (languageOptions?.math, languageOptions?.frontmatter) still necessary? (I initially implemented it to check for undefined.)
markdown/src/language/markdown-language.js
Line 171 in 3a8169c
markdown/src/language/markdown-language.js
Line 162 in 3a8169c
Initially, the motivation for introducing frontmatterOption !== undefined was in #328. At that time, the README.md stated that the minimum compatible ESLint version was any v9 release, so it made sense.
Later, we discovered that rules used defaultOptions, and we raised the minimum ESLint version to v9.15.0 (ref: #537).
The issue is that defaultLanguageOptions was introduced in ESLint v9.13.0. Since we now expect users to be running ESLint v9.15.0 or later, the frontmatter and math options can't be undefined.
markdown/src/language/markdown-language.js
Lines 134 to 136 in 3a8169c
However, currently there are no peerDependencies or other restrictions on the language plugins, so checking for undefined still seems sensible: some users might not depend on the rule and could only be using MarkdownSourceCode or MarkdownLanguage features with ESLint versions older than v9.13.0.
To summarize, for safety, would it be better to keep the undefined checks and optional chaining, or is it safe to remove them? If opening a separate issue would help, I'm happy to do that and work on it.
There was a problem hiding this comment.
I agree that we should keep the optional chaining on languageOptions and undefined checks until the next major version.
|
|
||
| ":matches(heading, paragraph, tableCell) :matches(html, image, imageReference, inlineCode, linkReference)"( | ||
| /** @type {Html | Image | ImageReference | InlineCode | LinkReference} */ node, | ||
| ":matches(heading, paragraph, tableCell) :matches(html, image, imageReference, inlineCode, linkReference, inlineMath)"( |
There was a problem hiding this comment.
This PR introduces two node types: math and inlineMath.
However, the math node is block-level, so it cannot be a child of a heading, paragraph or tableCell node. Only the inlineMath node can be a child of a heading, paragraph or tableCell, so I only added inlineMath to it.
| }); | ||
|
|
||
| // Validation tests for the `frontmatter` option | ||
| it("should throw an error if `frontmatter` is not `false`, `'yaml'`, `'toml'`, or `'json'`", () => { |
There was a problem hiding this comment.
The previous test logic for validateLanguageOptions() mixed frontmatter-specific tests with general tests, so I moved the general test suites to the top level. I also replaced the previous frontmatter error test that used a regex with a check against the full error message for better accuracy.
In summary, this diff here mainly restructures the test suites for improved clarity.
There was a problem hiding this comment.
Pull request overview
Adds optional Math (LaTeX) parsing support to @eslint/markdown via languageOptions.math, integrating micromark/mdast math extensions while keeping the default behavior unchanged (math parsing off).
Changes:
- Add
languageOptions.mathparsing support (CommonMark + GFM) usingmicromark-extension-mathandmdast-util-math. - Extend exported types and visitor typings to include
inlineMathandmathnodes. - Add/adjust tests and documentation for the new option, plus a small rule update to avoid false positives in math content.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/language/markdown-language.js |
Wires math: true/false into parser extensions and validates the option. |
src/types.ts |
Adds math?: boolean to MarkdownLanguageOptions and includes math node types in visitors. |
src/rules/no-reversed-media-syntax.js |
Ignores/masks inlineMath content when scanning for reversed media syntax. |
tests/language/markdown-language.test.js |
Verifies default “no math” parsing and “math enabled” parsing in both modes; updates validation assertions. |
tests/types/types.test.ts |
Ensures TypeScript surface area exposes MarkdownLanguageOptions and math node visitor selectors/types. |
tests/rules/no-space-in-emphasis.test.js |
Adds coverage ensuring math content doesn’t trigger emphasis-spacing checks when math parsing is enabled. |
tests/rules/no-reversed-media-syntax.test.js |
Adds a math-related valid case to prevent false positives. |
tests/rules/no-missing-link-fragments.test.js |
Adds coverage for heading/link fragment behavior when InlineMath appears in headings; also cleans up whitespace. |
tests/rules/no-missing-label-refs.test.js |
Adds coverage to ensure math content containing [] doesn’t interfere with label/reference detection. |
package.json |
Adds dependencies needed for math parsing (mdast-util-math, micromark-extension-math). |
README.md |
Documents languageOptions.math and notes the internal parser extensions used. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
src/language/markdown-language.js
Outdated
|
|
||
| if (mathOption !== undefined && typeof mathOption !== "boolean") { | ||
| throw new Error( | ||
| `Invalid language option value \`${mathOption}\` for math.`, |
There was a problem hiding this comment.
This error message should be more helpful (I know it has the same message as the error message for frontmatter).
I think we should update both error messages to include the expected values, e.g. "Invalid language option `${mathOption}` for math. Expected a boolean".
There was a problem hiding this comment.
Sounds good to me. I've updated it in c7c38ed.
FYI: I also looked into the error message formats used by the JSON and CSS plugins, but it seems there isn’t a common template.
|
Changes LGTM except the note about the error message. |
|
Friendly ping @DMartens |
Prerequisites checklist
What is the purpose of this pull request?
This PR supports the
mathoption as described in #331.I've reviewed all of the current rule implementations. The only affected rule was
no-reversed-media-syntax. I've added test cases to ensure it works. I also added some tests forno-missing-link-fragmentsbecause I suspected this change might affect it. However, there was no impact, so I only added tests. The newly added test cases also include the examples that were mentioned in #331 as false positives.I've left separate comments on several notable or questionable parts of this PR.
What changes did you make? (Give an overview)
This PR supports the
mathoption as described in #331.Related Issues
Closes: #331
Ref: #328
Is there anything you'd like reviewers to focus on?
N/A