test(markdown): add minimal service plumbing for formatter integration#9070
Conversation
|
Merging this PR will not alter performance
Comparing Footnotes
|
|
Caution Review failedFailed to post review comments WalkthroughThis PR introduces Markdown language support to the Biome ecosystem alongside significant enhancements to CSS and SCSS parsing and formatting. It adds a new Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/biome_service/Cargo.toml`:
- Line 74: The manifest uses an explicit version for the notify dependency
instead of the workspace reference; update the crate's Cargo.toml so the notify
dependency references the workspace-level declaration (use workspace = true) or
move notify into the root workspace [dependencies] and keep workspace = true
here; locate the notify entry (notify = { version = "8.2.0", features =
["crossbeam-channel"] }) and change it to a workspace reference consistent with
other dependencies or add notify to the workspace dependencies table and remove
the explicit version here.
🧹 Nitpick comments (2)
crates/biome_service/src/file_handlers/markdown.rs (1)
210-225: Parser metadata is discarded during AnyParse conversion.
parse_markdown_with_cachereturnslist_tightness,list_item_indents, andquote_indentsalongside the syntax tree (seecrates/biome_markdown_parser/src/lib.rs). These are lost when converting toAnyParseviaNodeParse. This is fine for the current minimal plumbing, but will need addressing when substantive formatting logic lands — list tightness in particular affects Markdown rendering semantics.crates/biome_service/src/file_handlers/mod.rs (1)
434-441:can_contain_embedsreturnsfalsefor Markdown — worth a TODO?Markdown files commonly contain fenced code blocks (e.g.,
```js ... ```), which are semantically embedded snippets. Returningfalseis fine for this minimal plumbing PR, but it might be worth leaving a short comment or TODO so this isn't forgotten when embedded-language support lands.Optional: leave a breadcrumb
Self::Html(_) | Self::Js(_) => true, Self::Css(_) | Self::Graphql(_) | Self::Json(_) | Self::Grit(_) + // TODO: Markdown fenced code blocks may contain embeds — revisit when embedded support lands. | Self::Markdown(_) | Self::Ignore | Self::Unknown => false,
| @@ -0,0 +1,267 @@ | |||
| use super::{ | |||
There was a problem hiding this comment.
Hi @jfmcdowell . Sorry I'm afraid this will conflict with #9067 soon. :(
I was having this implementation in my local so I can verify the snapshots are correctly captured in the tests. I'm fixing some errors before pushing it to my PR.
There was a problem hiding this comment.
FYI the reason I needed to implement what you had in this PR is because the tests need to have a TestFormatLanguage implementation which then requires ServiceLanguage implementation. So it overlaps with what you're doing in the PR.
ematipico
left a comment
There was a problem hiding this comment.
It looks good, however we need to gate the implementation under a cargo feature or Biome will start parsing the files
|
|
||
| pub fn try_from_extension(extension: &str) -> Result<Self, FileSourceError> { | ||
| match extension { | ||
| "md" | "markdown" => Ok(Self::markdown()), |
There was a problem hiding this comment.
This will enable handling markdown to our users, and we're not ready for this yet. To prevent this, do the same thing I did for SCSS in this PR: #9091
|
|
||
| pub fn try_from_language_id(language_id: &str) -> Result<Self, FileSourceError> { | ||
| match language_id { | ||
| "markdown" => Ok(Self::markdown()), |
There was a problem hiding this comment.
Same thing here, let's gate it under a cargo feature
| #[derive( | ||
| Debug, Clone, Default, Copy, Eq, PartialEq, Hash, serde::Serialize, serde::Deserialize, | ||
| )] | ||
| pub struct MarkdownFileSource { |
There was a problem hiding this comment.
nit: I would call it MdFileSource, just because it's shorter
|
@ematipico given @tidefield's comment on #9067 do you want me to hold off on this? |
To be honest, I lack the context of the comment. I don't know what that's about. To me, this PR seems fine cc @tidefield |
#9067 and #9076 mention comments about potential clashes. I think you (@ematipico) gave a path forward but I want to make sure we are all aligned. Cc @tidefield |
|
Hi @jfmcdowell. Given @ematipico 's comment in #9076 (comment), I think we can prioritize your PR first (switching it to base on main and addressing the comments) I'll rebase my branch to main after your PR is submitted. 🎉 |
c9228e0 to
f66e03d
Compare
Add the minimum markdown service integration so markdown is recognized as a formattable language and formatter tests can run against the standard service path. - Add MarkdownFileSource detection for md/markdown extensions and markdown language id. - Add DocumentFileSource::Markdown routing and markdown handler capabilities in biome_service. - Add markdown ServiceLanguage settings/format option resolution and formatter entry points. - Add MarkdownFormatOptions Display + missing option builder methods required by service plumbing. - Add focused service tests for markdown file-source detection and formatter capability routing. - Add markdown exhaustive match handling in xtask rules_check. Scope intentionally excludes Prettier markdown comparison tests (tracked separately).
- Added an experimental-markdown cargo feature to biome_markdown_syntax and gated Markdown detection in MdFileSource for: - file extensions (md, markdown) - language ID (markdown) - Renamed MarkdownFileSource to MdFileSource per maintainer feedback and updated all service/syntax references. - Updated biome_service markdown file-source capability tests to assert default feature-off behavior (Markdown stays Unknown unless the feature is enabled).
f66e03d to
e676f78
Compare
e676f78 to
e512804
Compare
Note
AI Assistance Disclosure: This PR was developed with assistance from Claude Code.
Summary
This PR adds the minimum Markdown formatter plumbing needed so Markdown is recognized as a formattable language in Biome.
PR #8962 introduced formatter boilerplate. This PR focuses on the required syntax/service/formatter-option integration to make Markdown format-capable from the service layer.
Test corpus and comparison/snapshot-heavy formatter tests are handled separately in #9067
Continues work on #3718
Why
Markdown must be recognized by
biome_serviceas a format-capable language before formatter infrastructure can be exercised end-to-end.Scope
1) Markdown file source + service routing
MarkdownFileSourcewith:md,markdownmarkdownDocumentFileSource::Markdownsupport in service routing/capability dispatch.biome_servicewith:2) Markdown formatter options plumbing
MarkdownFormatOptions(required by service language traitbounds).
with_indent_stylewith_indent_widthwith_line_endingwith_trailing_newline3) Service/settings integration
LanguageListSettings.biome_serviceCargo config.4) Exhaustive match maintenance
DocumentFileSource::Markdownarm inxtask/rules_checkfor exhaustive handling.5) Follow-up consistency cleanup in this PR
DocumentFileSource::to_markdown_file_source()accessor for parity with other file source helpers.Not Included
Those tests/corpus are being developed in #9067
Validation
just fjust l