feat(markdown): add prettier tests for markdown formatter#9076
feat(markdown): add prettier tests for markdown formatter#9076ematipico merged 17 commits intobiomejs:mainfrom
Conversation
|
Merging this PR will not alter performance
Comparing Footnotes
|
b543045 to
d4e4855
Compare
|
@ematipico given you already reviewed #9070 (review) and the PR overlaps with the plumbing I needed to do to get the tests running, feel free to defer reviewing my PR if you like. We can also just merge this PR (if it's approved sooner) and rebase @jfmcdowell 's branch in the other branch. I'm happy either way. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR introduces Markdown configuration support by creating a new Possibly related PRs
🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/biome_markdown_formatter/src/context.rs`:
- Around line 112-115: The Display impl for MarkdownFormatOptions currently
calls todo!() which will panic at runtime; replace the todo with a minimal,
non-panicking implementation (e.g., accept the fmt::Formatter parameter name f
and call write!(f, "{:?}", self) or write!(f, "MarkdownFormatOptions")) so
formatting uses the existing Debug derive or a simple placeholder; update the
impl for fmt::Display for MarkdownFormatOptions to use f and write the output
instead of todo!().
🧹 Nitpick comments (2)
crates/biome_markdown_formatter/tests/spec_tests.rs (1)
3-9: Placeholder test suite — no specs will run yet.The
tests/specs/markdown/directory doesn't exist, sogen_tests!will generate zero test functions. The TODO is noted, but it would be good to track this as an issue so it doesn't get forgotten.Would you like me to open an issue to track adding the markdown spec test fixtures?
crates/biome_configuration/src/markdown.rs (1)
20-22: Unused type aliases.
MarkdownLinterEnabled,MarkdownAssistEnabled, andMarkdownParseInterpolationare defined but not referenced anywhere in this PR. They're presumably scaffolding for future work — consider adding a brief comment to that effect, or removing them until needed.
308c131 to
b37d485
Compare
|
@tidefield I see now. Yes, there's a bit of overlap but @jfmcdowell PR's add things that are missing here. There's also the PRs target two different branches. Since we aren't enabling to the end user, it's safe to merge things in My advice:
|
b37d485 to
a2a74bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_configuration/src/lib.rs`:
- Line 38: The crate root currently imports MarkdownConfiguration and
markdown_configuration privately (use crate::markdown::{MarkdownConfiguration,
markdown_configuration}); change this to re-export them publicly so they follow
the same pattern as the other languages: replace the private import with a pub
use of MarkdownConfiguration and markdown_configuration at the crate root (i.e.,
add them to the existing pub use group alongside CssConfiguration,
GraphqlConfiguration, GritConfiguration, HtmlConfiguration, JsConfiguration,
JsonConfiguration) so downstream consumers can access
biome_configuration::MarkdownConfiguration and
biome_configuration::markdown_configuration directly.
In `@crates/biome_configuration/src/markdown.rs`:
- Line 24: Fix the grammar in the doc comment for the Options type: change the
phrase "Options that changes how the Markdown formatter behaves" to "Options
that change how the Markdown formatter behaves" (update the doc comment above
the Options definition in markdown.rs).
- Line 45: Update the doc comment "The size of the indentation applied to
Markdown files. Default to 2." to use correct grammar—change "Default to 2" to
"Defaults to 2"; this is the doc comment for the indentation field immediately
above the `line_width` field in markdown.rs so adjust that comment text
accordingly.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/biome_configuration/src/markdown.rs (2)
50-50: Nit: doc comment phrasing."What's the max width of a line" reads a bit colloquially for API docs. A minor tweak for consistency with typical biome doc style:
📝 Suggested wording
- /// What's the max width of a line applied to Markdown files. Defaults to 80. + /// The max width of a line applied to Markdown files. Defaults to 80.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_configuration/src/markdown.rs` at line 50, Replace the colloquial doc comment "What's the max width of a line applied to Markdown files. Defaults to 80." with a more formal API-style wording such as "Maximum line width for Markdown files. Defaults to 80." — update the doc comment attached to the Markdown configuration field (the comment currently reading "What's the max width of a line applied to Markdown files. Defaults to 80.") so it uses the suggested phrasing for consistency with other biome docs.
19-22: Three type aliases are unused and should be deferred.
MarkdownLinterEnabled,MarkdownAssistEnabled, andMarkdownParseInterpolationare defined but not used anywhere in the codebase. Introduce them only when the corresponding configuration fields are actually implemented.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_configuration/src/markdown.rs` around lines 19 - 22, Remove or defer the unused type aliases MarkdownLinterEnabled, MarkdownAssistEnabled, and MarkdownParseInterpolation from markdown.rs: they are defined but not referenced—leave only MarkdownFormatterEnabled for now (or keep as the only enabled/disabled experimental flag) and add the other aliases back only when you implement the corresponding config fields or features that reference MarkdownLinterEnabled, MarkdownAssistEnabled, and MarkdownParseInterpolation (ensure their definitions are reintroduced alongside the functions/structs that use them).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/biome_configuration/src/markdown.rs`:
- Line 50: Replace the colloquial doc comment "What's the max width of a line
applied to Markdown files. Defaults to 80." with a more formal API-style wording
such as "Maximum line width for Markdown files. Defaults to 80." — update the
doc comment attached to the Markdown configuration field (the comment currently
reading "What's the max width of a line applied to Markdown files. Defaults to
80.") so it uses the suggested phrasing for consistency with other biome docs.
- Around line 19-22: Remove or defer the unused type aliases
MarkdownLinterEnabled, MarkdownAssistEnabled, and MarkdownParseInterpolation
from markdown.rs: they are defined but not referenced—leave only
MarkdownFormatterEnabled for now (or keep as the only enabled/disabled
experimental flag) and add the other aliases back only when you implement the
corresponding config fields or features that reference MarkdownLinterEnabled,
MarkdownAssistEnabled, and MarkdownParseInterpolation (ensure their definitions
are reintroduced alongside the functions/structs that use them).
ddd3e25 to
4604a04
Compare
|
@ematipico this is ready for review now |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/biome_markdown_formatter/tests/prettier_tests.rs (1)
22-24:with_indent_width(IndentWidth::default())is redundant.
MarkdownFormatOptions::default()already initialisesindent_widthtoIndentWidth::default(), so the explicit call is a no-op — it just adds noise to the chain.♻️ Proposed simplification
- let options = MarkdownFormatOptions::default() - .with_indent_style(IndentStyle::Space) - .with_indent_width(IndentWidth::default()); + let options = MarkdownFormatOptions::default() + .with_indent_style(IndentStyle::Space);And you can drop the now-unused
IndentWidthimport:-use biome_formatter::{IndentStyle, IndentWidth}; +use biome_formatter::IndentStyle;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_markdown_formatter/tests/prettier_tests.rs` around lines 22 - 24, Remove the redundant with_indent_width(IndentWidth::default()) call from the options chain since MarkdownFormatOptions::default() already sets indent_width to IndentWidth::default(); update the code to just call MarkdownFormatOptions::default().with_indent_style(IndentStyle::Space) and also remove the now-unused IndentWidth import to keep imports clean.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/biome_markdown_formatter/tests/prettier_tests.rs`:
- Around line 22-24: Remove the redundant
with_indent_width(IndentWidth::default()) call from the options chain since
MarkdownFormatOptions::default() already sets indent_width to
IndentWidth::default(); update the code to just call
MarkdownFormatOptions::default().with_indent_style(IndentStyle::Space) and also
remove the now-unused IndentWidth import to keep imports clean.
| /// Specific configuration for the Markdown language | ||
| #[bpaf(external(markdown_configuration), optional)] | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub markdown: Option<MarkdownConfiguration>, |
There was a problem hiding this comment.
We can't change the configuration. The configuration is public, and it will show up to users the moment we cut a release.
There was a problem hiding this comment.
I removed it.
But what's the concern here? Per #9070 (comment), I assume that users can't use markdown formatter so any markdown configuration is no-op.
There was a problem hiding this comment.
It still makes it public, and it affects our generated schema. People will see it in their autocomplete and get confused on what the status of markdown is, or they will try it an mistakenly assume it works when it doesn't.
There was a problem hiding this comment.
Look at this commit 51fddbe
The hidden files are schema and workspace types. They become public. We expose an option to users (only the schema) that does nothing. Users will start to use suggestions in their IDE (due to autocompletion of the schema).
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
ca10e52 to
df62e73
Compare
Summary
This PR adds the prettier tests to run against the snapshots added by #9067.
Test Plan
Related #3718