chore(doc): update formatter contributing doc#8972
Conversation
|
|
Caution Review failedThe pull request is closed. WalkthroughReplaces the manual, example-driven CONTRIBUTING.md for crates/biome_formatter with a codegen-first workflow. Documents a multi-step process: create a formatter crate, run the generator to produce boilerplate, inspect generated files (lib.rs, context.rs, comments.rs, cst.rs, prelude.rs, verbatim.rs, language modules, generated.rs), add required types, implement node-specific formatters, and run generated tests. Emphasises that the .ungram grammar remains the source of truth and that codegen maps grammar AST nodes to files and trait implementations. No public API changes. 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/biome_formatter/CONTRIBUTING.md`:
- Line 332: Change the phrase "Accept snapshots after verifying they're
correct:" in CONTRIBUTING.md to use the possessive pronoun; replace it with
"Accept snapshots after verifying their correctness:" so the sentence reads
correctly, updating the exact string found in the file.
- Line 15: Add a missing comma after "example" in the CONTRIBUTING.md sentence
that reads "The following steps refer to html as an example but it should be
mostly relevant for any language." to improve readability; locate the sentence
in CONTRIBUTING.md (the paragraph referencing "html as an example") and insert a
comma so it reads "html as an example, but it should be mostly relevant for any
language."
| } | ||
| } | ||
| } | ||
| The recommended way to add a formatter for a new language is to use provided codegen This automatically generates the boilerplate code based on the language's grammar. The following steps refer to html as an example but it should be mostly relevant for any language. |
There was a problem hiding this comment.
Add the missing comma for readability.
“...as an example, but it should...” reads better and avoids the run‑on.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~15-~15: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...lowing steps refer to html as an example but it should be mostly relevant for any la...
(COMMA_COMPOUND_SENTENCE)
🤖 Prompt for AI Agents
In `@crates/biome_formatter/CONTRIBUTING.md` at line 15, Add a missing comma after
"example" in the CONTRIBUTING.md sentence that reads "The following steps refer
to html as an example but it should be mostly relevant for any language." to
improve readability; locate the sentence in CONTRIBUTING.md (the paragraph
referencing "html as an example") and insert a comma so it reads "html as an
example, but it should be mostly relevant for any language."
| cargo t | ||
| ``` | ||
|
|
||
| Accept snapshots after verifying they're correct: |
There was a problem hiding this comment.
Use “their” here.
“Accept snapshots after verifying their correctness” is the intended phrasing.
🧰 Tools
🪛 LanguageTool
[misspelling] ~332-~332: Did you mean “verifying their”?
Context: ...ell cargo t Accept snapshots after verifying they're correct: shell cargo insta accept `...
(VBG_THEYRE)
🤖 Prompt for AI Agents
In `@crates/biome_formatter/CONTRIBUTING.md` at line 332, Change the phrase
"Accept snapshots after verifying they're correct:" in CONTRIBUTING.md to use
the possessive pronoun; replace it with "Accept snapshots after verifying their
correctness:" so the sentence reads correctly, updating the exact string found
in the file.
ematipico
left a comment
There was a problem hiding this comment.
First review, I'll continue later
| This guide assumes the parser and grammar for your language are already implemented by following [the guidance](../biome_parser/CONTRIBUTING.md) | ||
|
|
||
| <summary>Code to copy</summary> | ||
| The formatter codegen depends on the generated AST types from the grammar, so you must complete the parser and grammar implementation before starting on the formatter. |
There was a problem hiding this comment.
I would avoid the acronym AST. Biome uses CST. Maybe use nodes/tokens or types
There was a problem hiding this comment.
I was wondering about this as well. Why does the generated code refer to biome_rowan::AstNode though?
There was a problem hiding this comment.
Probably poor naming convention from rowan. Or maybe because it's the red node, which "mimics" an AST like navigation with typed nodes.
Still, it's not an AST, because we have access to trivia.
Co-authored-by: Emanuele Stoppa <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/biome_formatter/CONTRIBUTING.md`:
- Around line 240-242: Remove the stray leading punctuation character before the
first bullet in the list describing format traits and ensure consistent bullet
punctuation; specifically edit the list containing **AsFormat<Context>**,
**IntoFormat<Context>**, and **FormatRule<NodeType>** in CONTRIBUTING.md to
remove the odd character preceding the first bullet and make the bullets'
punctuation consistent (either remove terminal punctuation from all or add
periods to all).
- Around line 236-243: Update the trait name in the "Key traits created by
codegen" list from FormatRule<NodeType> to FormatNodeRule<NodeType]; mention
that codegen implements the FormatNodeRule<N> trait and that the required method
is fmt_fields while default implementations exist for fmt, is_suppressed,
fmt_leading_comments, fmt_dangling_comments, and fmt_trailing_comments to avoid
confusion for contributors.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/biome_formatter/CONTRIBUTING.md`:
- Line 314: The test command in CONTRIBUTING.md contains a typo: change the test
name invocation "html::simle_element" to "html::simple_element" so documentation
and test references match the actual test; update any occurrences of the
misspelled "simle_element" to "simple_element" (e.g., the example cargo test
invocation and any referenced test function names).
023e5f3 to
d04b6f1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/biome_formatter/CONTRIBUTING.md`:
- Around line 29-40: Change the CONTRIBUTING wording to state that the generator
produces node-specific implementations of the FormatNodeRule trait (e.g.,
FormatNodeRule<N>) rather than generic FormatRule implementations: explain that
generated impls implement FormatNodeRule<N> and must provide fmt_fields, while
fmt, is_suppressed, fmt_leading_comments, fmt_dangling_comments, and
fmt_trailing_comments have default implementations; update the bullet that
currently reads "Generates default `FormatRule` implementations" to mention
`FormatNodeRule<N>` and `fmt_fields` as the required method to reflect the
actual generated symbols.
f23529d to
fe90dc6
Compare
ematipico
left a comment
There was a problem hiding this comment.
That's a great improvement. Thank you @tidefield for the enhancements!
fe90dc6 to
e3eccb9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/biome_formatter/CONTRIBUTING.md`:
- Around line 104-106: The example implements the wrong concrete type: replace
the impl block that reads impl FormatOptions for MarkdownFormatOptions with impl
FormatOptions for HtmlFormatOptions so the trait implementation matches the HTML
formatter context; update any example mentions of MarkdownFormatOptions to
HtmlFormatOptions to avoid the copy-paste mismatch for the FormatOptions trait.
6133bd5 to
45c28bd
Compare
45c28bd to
4a4ddc6
Compare
My pleasure! As always, thanks for the thorough review. It helps me re-learn how things work once again. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/biome_formatter/CONTRIBUTING.md`:
- Line 333: The relative link text "../crates/biome_parser/CONTRIBUTING.md" in
the sentence about "Undefined node: AnyHtmlBlock" is incorrect; update that path
to "../biome_parser/CONTRIBUTING.md" to match the correct link used elsewhere
(see the existing correct reference at line 9) so the parser contributing guide
resolves properly.
🧹 Nitpick comments (1)
crates/biome_formatter/CONTRIBUTING.md (1)
159-179: Document the constructor forHtmlFormatLanguage.The code at line 179 calls
HtmlFormatLanguage::new(options), but thenewmethod isn't shown in any of the earlier code examples. Contributors following this guide won't know how to implement it.📋 Suggested addition after line 161
pub struct HtmlFormatLanguage { options: HtmlFormatOptions, } + +impl HtmlFormatLanguage { + pub fn new(options: HtmlFormatOptions) -> Self { + Self { options } + } +}
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
I was working on #8962 and confused by the organization of the contributing doc. It hasn't been updated for a while.
The PR fixes and improves the readability to better reflect how the codegen actually works (for easier debugging if something is missing or has error) and what we need to implement manually.
Disclosure: I used AI to help putting up the improvement but I manually go through each step carefully to make sure it's readable.