feat(formatter): set up boiletplate for markdown formatter#8962
feat(formatter): set up boiletplate for markdown formatter#8962ematipico merged 15 commits intobiomejs:nextfrom
Conversation
|
github-actions
bot
added
A-Formatter
| type Format<'a> = FormatRefWithRule< | ||
| 'a, | ||
| biome_markdown_syntax::AnyCodeBlock, | ||
| crate::js::any::block::FormatAnyCodeBlock, |
There was a problem hiding this comment.
Compile error:
error[E0425]: cannot find type `FormatAnyCodeBlock` in module `crate::js::any::block`
The generated code misses this. I'm still looking into it but would love to get second pair of eyes on this.
There was a problem hiding this comment.
It's an error in the grammar I think. We have a few nodes that aren't called AnyMd*. The Md is missing from the name
There was a problem hiding this comment.
Biome has great infrastructure to catch bug like this.
CodSpeed Performance ReportMerging this PR will not alter performanceComparing Summary
Footnotes
|
WalkthroughAdds a new workspace crate Possibly related PRs
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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@crates/biome_markdown_formatter/Cargo.toml`:
- Around line 12-28: Update Cargo.toml so test-only crates are under
[dev-dependencies]: add biome_formatter_test = { workspace = true } and move
countme, iai, quickcheck, quickcheck_macros into [dev-dependencies]; also
relocate biome_markdown_parser, and consider moving biome_parser and
biome_service from [dependencies] to [dev-dependencies] (they are only used in
tests) so that [dependencies] only contains runtime items like biome_rowan,
biome_formatter, biome_markdown_factory, and biome_markdown_syntax.
In `@crates/biome_markdown_formatter/src/comments.rs`:
- Around line 17-19: The function is_suppression currently always returns true,
causing every comment to be treated as a suppression; change is_suppression(_:
&str) -> bool to return false by default (i.e., return false instead of true) so
suppression detection does not erroneously mark all comments, and optionally add
a TODO comment in the is_suppression function indicating where to implement real
suppression detection logic later.
In `@crates/biome_markdown_formatter/src/context.rs`:
- Around line 89-91: The method source_map currently always returns None; change
it to return the stored value set by with_source_map by returning
self.source_map.as_ref() (i.e., return an Option<&TransformSourceMap> that
references the existing self.source_map field) so callers see the actual
TransformSourceMap that was previously stored.
In `@crates/biome_markdown_formatter/src/lib.rs`:
- Around line 217-220: The current impl of FormatNodeRule::fmt for the type
implementing fmt in lib.rs uses todo!(), causing a panic; replace it with the
standard delegation pattern used by other formatters: check and apply
suppression via the formatter (use suppress logic), format leading comments,
call the node-specific fmt_fields(&self, node, formatter) to format the node
body, then format trailing comments and return the FormatResult; locate the
fmt(&self, _: &N, __: &mut MarkdownFormatter) method and implement the
suppression + leading-comments -> fmt_fields -> trailing-comments sequence
consistent with other formatters (refer to their implementations of
FormatNodeRule::fmt for exact call order and helper names).
In `@crates/biome_markdown_formatter/src/verbatim.rs`:
- Around line 19-23: The markdown verbatim formatter
(FormatMarkdownVerbatimNode) never emits the StartVerbatim tag, leaving the kind
field unused; update the Format implementation for FormatMarkdownVerbatimNode to
call f.write_element(FormatElement::Tag(Tag::StartVerbatim(self.kind)))?; before
emitting the verbatim content so it matches the pattern used by other formatters
(e.g., CSS/HTML) and ensures the StartVerbatim tag is written; reference the
struct FormatMarkdownVerbatimNode and its kind field and add the write_element
call at the beginning of the formatting routine for that node.
🧹 Nitpick comments (14)
crates/biome_markdown_formatter/src/markdown/bogus/bogus.rs (1)
1-5: Add a brief rustdoc for this formatter rule.
Keeps formatter rules consistent and discoverable.Suggested patch
use crate::FormatBogusNodeRule; use biome_markdown_syntax::MdBogus; #[derive(Debug, Clone, Default)] +/// Formats bogus Markdown nodes. pub(crate) struct FormatMdBogus; impl FormatBogusNodeRule<MdBogus> for FormatMdBogus {}As per coding guidelines: Use inline rustdoc documentation for rules, assists, and their options.
crates/biome_markdown_formatter/src/markdown/auxiliary/textual.rs (1)
1-10: Add a short rustdoc for the formatter rule.
Small docs win.Suggested patch
use crate::prelude::*; use biome_markdown_syntax::MdTextual; use biome_rowan::AstNode; #[derive(Debug, Clone, Default)] +/// Formats Markdown textual nodes verbatim. pub(crate) struct FormatMdTextual; impl FormatNodeRule<MdTextual> for FormatMdTextual { fn fmt_fields(&self, node: &MdTextual, f: &mut MarkdownFormatter) -> FormatResult<()> { format_verbatim_node(node.syntax()).fmt(f) } }As per coding guidelines: Use inline rustdoc documentation for rules, assists, and their options.
crates/biome_markdown_formatter/src/markdown/auxiliary/inline_emphasis.rs (1)
1-10: Please add a brief rustdoc comment.
Consistency makes future spelunking easier.Suggested patch
use crate::prelude::*; use biome_markdown_syntax::MdInlineEmphasis; use biome_rowan::AstNode; #[derive(Debug, Clone, Default)] +/// Formats Markdown inline emphasis nodes verbatim. pub(crate) struct FormatMdInlineEmphasis; impl FormatNodeRule<MdInlineEmphasis> for FormatMdInlineEmphasis { fn fmt_fields(&self, node: &MdInlineEmphasis, f: &mut MarkdownFormatter) -> FormatResult<()> { format_verbatim_node(node.syntax()).fmt(f) } }As per coding guidelines: Use inline rustdoc documentation for rules, assists, and their options.
crates/biome_markdown_formatter/src/markdown/lists/bullet_list.rs (1)
1-9: Add a short rustdoc note for this rule.
Helps keep formatter rules documented.Suggested patch
use crate::prelude::*; use biome_markdown_syntax::MdBulletList; #[derive(Debug, Clone, Default)] +/// Formats Markdown bullet lists. pub(crate) struct FormatMdBulletList; impl FormatRule<MdBulletList> for FormatMdBulletList { type Context = MarkdownFormatterContext; fn fmt(&self, node: &MdBulletList, f: &mut MarkdownFormatter) -> FormatResult<()> { f.join().entries(node.iter().formatted()).finish() } }As per coding guidelines: Use inline rustdoc documentation for rules, assists, and their options.
crates/biome_markdown_formatter/src/markdown/auxiliary/inline_image_alt.rs (1)
4-6: Add a brief rustdoc line for this formatter rule.
A single-line summary is enough and keeps the rules catalogue tidy.✍️ Suggested tweak
-#[derive(Debug, Clone, Default)] +/// Formats `MdInlineImageAlt` verbatim. +#[derive(Debug, Clone, Default)] pub(crate) struct FormatMdInlineImageAlt;As per coding guidelines: Use inline rustdoc documentation for rules, assists, and their options.
crates/biome_markdown_formatter/src/markdown/auxiliary/header.rs (1)
4-6: Same rustdoc nudge here for the rule struct.
Keeps the formatter rule set well-documented.✍️ Suggested tweak
-#[derive(Debug, Clone, Default)] +/// Formats `MdHeader` verbatim. +#[derive(Debug, Clone, Default)] pub(crate) struct FormatMdHeader;As per coding guidelines: Use inline rustdoc documentation for rules, assists, and their options.
crates/biome_markdown_formatter/src/markdown/auxiliary/quote.rs (1)
4-6: Add a short rustdoc line for this formatter rule too.
Keeps the set consistent and discoverable.✍️ Suggested tweak
-#[derive(Debug, Clone, Default)] +/// Formats `MdQuote` verbatim. +#[derive(Debug, Clone, Default)] pub(crate) struct FormatMdQuote;As per coding guidelines: Use inline rustdoc documentation for rules, assists, and their options.
crates/biome_markdown_formatter/src/markdown/auxiliary/link_block.rs (1)
4-6: Quick rustdoc note for the rule struct, please.✍️ Suggested tweak
-#[derive(Debug, Clone, Default)] +/// Formats `MdLinkBlock` verbatim. +#[derive(Debug, Clone, Default)] pub(crate) struct FormatMdLinkBlock;As per coding guidelines: Use inline rustdoc documentation for rules, assists, and their options.
crates/biome_markdown_formatter/src/markdown/lists/code_name_list.rs (1)
3-5: Add a one-liner rustdoc for this formatter rule.✍️ Suggested tweak
-#[derive(Debug, Clone, Default)] +/// Formats `MdCodeNameList`. +#[derive(Debug, Clone, Default)] pub(crate) struct FormatMdCodeNameList;As per coding guidelines: Use inline rustdoc documentation for rules, assists, and their options.
crates/biome_markdown_formatter/src/markdown/lists/indented_code_line_list.rs (1)
3-5: Rustdoc line for the rule struct would be great.✍️ Suggested tweak
-#[derive(Debug, Clone, Default)] +/// Formats `MdIndentedCodeLineList`. +#[derive(Debug, Clone, Default)] pub(crate) struct FormatMdIndentedCodeLineList;As per coding guidelines: Use inline rustdoc documentation for rules, assists, and their options.
crates/biome_markdown_formatter/src/comments.rs (2)
25-31:place_commentcontainstodo!()— will panic at runtime.This is fine for initial scaffolding, but worth tracking. Ensure this is addressed before the formatter is used in production.
Would you like me to open an issue to track implementing proper comment placement logic?
39-45:FormatMarkdownLeadingComment::fmtcontainstodo!()— will panic at runtime.Same concern as above. Also, consider using
_finstead of__for the unused formatter parameter — it's more idiomatic Rust.✨ Minor naming tweak
fn fmt( &self, _: &SourceComment<MarkdownLanguage>, - __: &mut Formatter<Self::Context>, + _f: &mut Formatter<Self::Context>, ) -> FormatResult<()> { todo!(); }crates/biome_markdown_formatter/src/context.rs (1)
14-18: Naming convention:MarkdownFormatterContextvsMarkdownFormatContext.Other formatters use the
<Lang>FormatContextpattern (e.g.,JsFormatContext,CssFormatContext). This usesMarkdownFormatterContext. Not blocking, but worth considering for consistency across the codebase.Also applies to: 67-74
xtask/codegen/src/formatter.rs (1)
938-938: Naming inconsistency:MarkdownFormatterContextvs<Lang>FormatContextpattern.All other languages use
<Lang>FormatContext(e.g.,JsFormatContext,CssFormatContext), but Markdown usesMarkdownFormatterContext. This should align with the pattern for consistency.♻️ Suggested change for consistency
- Self::Markdown => "MarkdownFormatterContext", + Self::Markdown => "MarkdownFormatContext",Note: This would require renaming
MarkdownFormatterContexttoMarkdownFormatContextincontext.rsas well.
tidefield
changed the title
tidefield
force-pushed
the
md-formatter-setup
branch
from
3f40549 to
92b3fa4
Compare
tidefield
force-pushed
the
md-formatter-setup
branch
from
134db79 to
7dedd12
Compare
tidefield
changed the title
tidefield
changed the title
tidefield
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/biome_markdown_formatter/src/comments.rs`:
- Around line 39-45: The FormatMarkdownLeadingComment::fmt implementation
currently calls todo!() which will panic at runtime; replace the todo!() with a
no-op that returns Ok(()) to avoid panics for now (match the same safe
placeholder used in place_comment), keeping the function signature (fn
fmt(&self, _: &SourceComment<MarkdownLanguage>, _: &mut
Formatter<Self::Context>) -> FormatResult<()>) unchanged so proper formatting
can be implemented later.
- Around line 25-30: The method place_comment currently uses todo!() and will
panic; replace it by naming the parameter (remove the underscore) so you can use
the incoming biome_formatter::comments::DecoratedComment<Self::Language> value
and return a safe default like
biome_formatter::comments::CommentPlacement::Default(...) containing the
provided comment (or its inner comment) to avoid panic; keep a short TODO
comment indicating proper comment handling is pending if needed.
🧹 Nitpick comments (1)
crates/biome_markdown_formatter/src/comments.rs (1)
11-12: Consider adding rustdoc for public types.
MarkdownCommentStyleandFormatMarkdownLeadingCommentare public but lack documentation. A brief description helps future contributors understand their purpose.Also applies to: 33-34
tidefield
force-pushed
the
md-formatter-setup
branch
from
ed8c552 to
1974014
Compare
tidefield
force-pushed
the
md-formatter-setup
branch
from
cea2a8d to
2f7a6ae
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/biome_markdown_formatter/src/verbatim.rs`:
- Line 34: Fix the typo in the inline comment in verbatim.rs: change "by pass"
to the single-word "bypass" in the comment that reads "// to by pass
`assert_checked_all_suppressions`" so the comment becomes "// to bypass
`assert_checked_all_suppressions`" (update the string in the same location
within the verbatim.rs source).
🧹 Nitpick comments (1)
crates/biome_markdown_formatter/tests/quick_test.rs (1)
17-24: Fragile assertion on internal IR structure.This asserts on
FormatElement::Textinternals, which will break if/when the verbatim path is updated to emitStartVerbatim/EndVerbatimtags (as flagged in the past review onverbatim.rs). Consider asserting on the printed output instead:Suggested alternative
- let boxed_text: Box<str> = source.into(); - assert_eq!( - result.unwrap().document().as_elements().first().unwrap(), - &FormatElement::Text { - text: boxed_text, - source_position: TextSize::default(), - } - ); + let formatted = result.unwrap(); + let printed = formatted.print().expect("printing should succeed"); + assert_eq!(printed.as_code(), source);
Actually. @ematipico Should I base my work into Edit: Yeah I think I should! |
tidefield
force-pushed
the
md-formatter-setup
branch
from
e74b14e to
19ced8f
Compare
tidefield
force-pushed
the
md-formatter-setup
branch
from
1eef6e5 to
443a8a1
Compare
| fn fmt(&self, node: &AnyCssPageAtRuleItem, f: &mut CssFormatter) -> FormatResult<()> { | ||
| match node { | ||
| AnyCssPageAtRuleItem::CssAtRule(node) => node.format().fmt(f), | ||
| AnyCssPageAtRuleItem::CssBogus(node) => node.format().fmt(f), |
There was a problem hiding this comment.
I'm aware this is unrelated. It's auto-regenerated by the codegen. Let me know if you want it in a different PR.
|
@ematipico the PR is reviewable now. |
|
Thank you @tidefield , I just merged a PR, and you can see the conflicts. Could you resolve them, please? Apologies for that |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@ematipico no problem. I'm actually already resolving the conflicts. 😄 Will push in a few minutes |
tidefield
force-pushed
the
md-formatter-setup
branch
from
7b35f5f to
5889efe
Compare
| MdBlockList = AnyMdBlock* | ||
|
|
||
| AnyMdBlock = | ||
| AnyLeafBlock |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/biome_markdown_formatter/src/lib.rs`:
- Line 201: Update the documentation comment that currently reads "/// Rule for
formatting an bogus nodes." to correct the article and grammar by changing it to
"/// Rule for formatting a bogus node." (or "nodes" if plural is intended);
locate the doc comment string in lib.rs and fix the wording accordingly where
the rule's doc comment appears.
🧹 Nitpick comments (3)
crates/biome_markdown_formatter/src/markdown/auxiliary/indent.rs (1)
3-3: Unnecessaryuse biome_rowan::AstNodeimport.All sibling auxiliary formatters (e.g.,
hash.rs,bullet.rs,header.rs,document.rs, etc.) rely solely oncrate::prelude::*for theAstNodetrait — no explicit import needed. This file deviates from that pattern.Proposed fix
use crate::prelude::*; use biome_markdown_syntax::MdIndent; -use biome_rowan::AstNode;crates/biome_markdown_formatter/src/markdown/auxiliary/newline.rs (1)
3-3: Same unnecessaryuse biome_rowan::AstNodeimport as inindent.rs.Sibling formatters don't need this — the prelude covers it.
Proposed fix
use crate::prelude::*; use biome_markdown_syntax::MdNewline; -use biome_rowan::AstNode;crates/biome_markdown_formatter/src/lib.rs (1)
40-47: Unnecessary.clone()—selfis already consumed by value.Since
create_contexttakesselfby value,self.optionscan be moved directly.Suggested fix
fn create_context( self, _root: &MarkdownSyntaxNode, source_map: Option<TransformSourceMap>, _delegate_fmt_embedded_nodes: bool, ) -> MarkdownFormatContext { - MarkdownFormatContext::new(self.options.clone()).with_source_map(source_map) + MarkdownFormatContext::new(self.options).with_source_map(source_map) }
| fn format(&self) -> Self::Format<'_>; | ||
| } | ||
|
|
||
| /// Rule for formatting an bogus nodes. |
There was a problem hiding this comment.
Tiny typo: "an bogus" → "a bogus".
-/// Rule for formatting an bogus nodes.
+/// Rule for formatting a bogus node.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Rule for formatting an bogus nodes. | |
| /// Rule for formatting a bogus node. |
🤖 Prompt for AI Agents
In `@crates/biome_markdown_formatter/src/lib.rs` at line 201, Update the
documentation comment that currently reads "/// Rule for formatting an bogus
nodes." to correct the article and grammar by changing it to "/// Rule for
formatting a bogus node." (or "nodes" if plural is intended); locate the doc
comment string in lib.rs and fix the wording accordingly where the rule's doc
comment appears.
|
@ematipico I fixed the merge conflicts. |
| fn quick_test() { | ||
| let source = "A simple paragraph"; | ||
| let parse = parse_markdown(source); | ||
| let options = MarkdownFormatOptions::default(); | ||
|
|
||
| let result = | ||
| biome_formatter::format_node(&parse.syntax(), MarkdownFormatLanguage::new(options), false); | ||
|
|
||
| let boxed_text: Box<str> = source.into(); | ||
| assert_eq!( | ||
| result.unwrap().document().as_elements().first().unwrap(), | ||
| &FormatElement::Text { | ||
| text: boxed_text, | ||
| source_position: TextSize::default(), | ||
| } | ||
| ); | ||
| } |
There was a problem hiding this comment.
It should also check reformat, see other quick tests for other formatters
There was a problem hiding this comment.
As mentioned in the test comment, I don't really care whether the reformat is correct at this stage. This is only meant to test format_node runs without errors and that the text comes back complete.
The next PR will implement the true formatting logic and I'll include the formatting tests there.
Let me know if you meant something else.
There was a problem hiding this comment.
I took a look at other quick tests. I remember why I didn't have it now. It requires having TestFormatLanguage implementation which will require hooking up with biome_service. I chose not to do it here to keep the PR lean.
I'll do that in the next PR.
4 participants
Summary
Adding the boilerplate for Markdown formatter as part of #3718.
When I tried writing a simple test for a paragraph, I realized markdown parser isn't complete yet. In a follow-up PR, I'll get the parser to some completeness before continuing on the formatter.I'll bring the formatter to a more complete state in the next PR.Test Plan
Run
to verify the boilerplate works end-to-end.
Similar to #8000, I decided to leave creating the test language in a follow-up PR because it requires hooking up with
biome_service.