Conversation
🦋 Changeset detectedLatest commit: 6fe0aff The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
8b279bb to
f2e5e48
Compare
CodSpeed Performance ReportMerging #8334 will not alter performanceComparing Summary
Footnotes
|
|
Not a fan of the option, but if there is user demand and proper documentation, then I don’t mind indeed. |
|
Also considered naming the option |
|
I share @arendjr 's sentiment. The logic is simple enough, so it's not a huge deal I suppose. |
1b63a75 to
aee8160
Compare
WalkthroughAdds a configurable trailing-newline feature throughout the formatter stack: new public type Possibly related PRs
Suggested reviewers
Pre-merge checks✅ Passed checks (2 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
crates/biome_formatter_test/src/snapshot_builder.rs (1)
102-102: Inconsistent handling betweenwith_output_and_optionsandwith_output.The extra newline added here makes trailing newlines evident in snapshots, but the
with_outputmethod (line 115) doesn't include this same newline before its closing backticks. Should both methods format output consistently, or is there a specific reason for the difference?🔎 Suggested change for consistency
If both methods should format output the same way, apply this change to
with_output:self.write_extension(); self.snapshot.push_str(output.content); +writeln!(self.snapshot).unwrap(); writeln!(self.snapshot, "```").unwrap();crates/biome_cli/tests/cases/editorconfig.rs (1)
702-747: Test structure looks good.The test follows the established pattern and properly sets up editorconfig integration. Consider adding a comment explaining the expected behaviour (why
is_err()is expected wheninsert_final_newline = falseand the file has no trailing newline).crates/biome_js_formatter/src/js/auxiliary/module.rs (1)
32-40: Order inconsistency with HTML formatter.This implementation emits
format_removed(eof_token)before the conditionalhard_line_break, whilst the HTML formatter (incrates/biome_html_formatter/src/html/auxiliary/root.rslines 30-34) emits the conditionalhard_line_breakfirst, thenformat_removed(eof_token). Consider standardising the order for consistency across formatters, though functionally this may not matter for EOF handling.crates/biome_formatter/src/lib.rs (1)
1207-1215: Implementation is correct and handles all line ending types.The while loop approach is simple and works well for the typical case of 1-2 trailing newlines.
Optional: Minor optimization for edge cases
If you expect files with many trailing newlines, you could optimize by finding the last non-newline position and truncating once:
pub fn strip_trailing_newlines(mut self) -> Self { - // Strip all trailing line ending characters - while self.code.ends_with('\n') || self.code.ends_with('\r') { - self.code.pop(); - } + // Find the last non-newline position + let trim_pos = self.code + .trim_end_matches(|c| c == '\n' || c == '\r') + .len(); + self.code.truncate(trim_pos); self }However, the current implementation is perfectly fine for the common case.
|
It seems we're pretty aligned with the option. We don't love it, but its maintenance is trivial. |
Summary
This is a proposal to add
trailingNewlineto the Biome formatter.I would like to know what @biomejs/core-contributors and @biomejs/maintainers think about it.
I would plan to add this option as long as we document the unsafety of removing the trailing newline from the files. Hence, this will never be the default.
For example, there are various historical issues about files that don't have a newline:
A user also raised a concern regarding POSIX. However, this seems to be more nounced (I asked Claude Code about this)
Still, it seems to be a concern to acknowledge, and maybe document.
Here's what I was thinking:
Note
The majority of the code was created with Claude Code, but I to intervene too because at some point the AI didn't know how to connect the dots. The AI updated all the configs and tests.
I updated the snapshots to make the newline more evident, since it wasn't before. I wrote some gluecode to make the feature working via Workspace, and edited the changeset
Test Plan
New tests added.
Docs
biomejs/website#3771