Conversation
🦋 Changeset detectedLatest commit: c2686a6 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 |
github-actions
bot
added
A-CLI
WalkthroughThis PR replaces Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 1
🤖 Fix all issues with AI agents
In `@crates/biome_service/src/workspace.rs`:
- Around line 544-546: Update the doc comment for the enum variant NotRequested
to fix the typo: change "It's very much ignore [SupportKind::Ignored], but it's
silent" to "It's very much like [SupportKind::Ignored], but it's silent" so the
comment correctly references SupportKind::Ignored and reads naturally.
🧹 Nitpick comments (1)
crates/biome_cli/src/runner/execution.rs (1)
17-21: Minor doc clarification.The comment on line 20 says "should be disabled", but semantically these are features "not requested" by the command rather than features to actively disable. Consider rewording for clarity:
📝 Suggested doc improvement
/// The features that this command requires to be enabled. fn wanted_features(&self) -> FeatureName; - /// The features that this command should be disabled. + /// The features that this command does not request (and should be ignored during negotiation). fn not_requested_features(&self) -> FeatureName;
| /// Particular state used when a client (e.g. CLI) doesn't require a particular feature. | ||
| /// It's very much ignore [SupportKind::Ignored], but it's silent | ||
| NotRequested, |
There was a problem hiding this comment.
Minor typo in doc comment.
"It's very much ignore" should be "It's very much like". Quick fix:
📝 Suggested fix
- /// Particular state used when a client (e.g. CLI) doesn't require a particular feature.
- /// It's very much ignore [SupportKind::Ignored], but it's silent
+ /// Particular state used when a client (e.g. CLI) doesn't require a particular feature.
+ /// It's very much like [SupportKind::Ignored], but it's silent
NotRequested,📝 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.
| /// Particular state used when a client (e.g. CLI) doesn't require a particular feature. | |
| /// It's very much ignore [SupportKind::Ignored], but it's silent | |
| NotRequested, | |
| /// Particular state used when a client (e.g. CLI) doesn't require a particular feature. | |
| /// It's very much like [SupportKind::Ignored], but it's silent | |
| NotRequested, |
🤖 Prompt for AI Agents
In `@crates/biome_service/src/workspace.rs` around lines 544 - 546, Update the doc
comment for the enum variant NotRequested to fix the typo: change "It's very
much ignore [SupportKind::Ignored], but it's silent" to "It's very much like
[SupportKind::Ignored], but it's silent" so the comment correctly references
SupportKind::Ignored and reads naturally.
| 16 │ <style>#id { | ||
| > 17 │ font-family: comic-sans; | ||
| │ ^^^^^^^^^^ | ||
| 18 │ } | ||
| 19 │ .class { |
There was a problem hiding this comment.
Shouldn't this have a newline before the #id?
There was a problem hiding this comment.
Good catch, that's a regression
There was a problem hiding this comment.
So the issue is actually bigger, and it's not a regression. With the fix we simply spotted the error that already exists
I know what's causing it, however the solution isn't clear to me yet.
I suggest merging the PR and I'll look after this in another PR. What do you think?
There was a problem hiding this comment.
IMO, it would block the release of 2.4, because it still looks like a regression from the end users perspective.
Either:
- Merge this, create a new issue to track this, and add it to the 2.4 milestone
- Hold off until 2.4 is out
There was a problem hiding this comment.
Yes that's one way to look at it, but also remember that this under the umbrella of the experimental full support, so regressions/mistakes/bugs/breaking changes are completely normal and under the "experimental contract". So I wouldn't be too worried about it.
There was a problem hiding this comment.
A fix I have in mind, that doesn't require too much hassle, would be to change when diagnostics are pulled.
At the moment, when running check with --write this happens:
- apply fixes
- pull diagnostics
- format
The change envision that would fix the problem is the following
- apply fixes
- format
- pull diagnostics
There was a problem hiding this comment.
I just think it would be unfortunate to ship it in this state.
There was a problem hiding this comment.
That should be fixed now in 9f6156d (this PR)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/biome_cli/src/runner/impls/process_file/lint_and_assist.rs`:
- Around line 95-100: The code left a dbg!(&output) call inside the
change-detection block (where output, input, changed, and
workspace_file.update_file are used); remove the dbg!(&output) line before
merging so debug stderr output isn't emitted in production — if you need runtime
logging instead, replace it with the project's logger (e.g., use the existing
logging facility) rather than dbg!.
🧹 Nitpick comments (1)
crates/biome_service/src/file_handlers/html.rs (1)
1144-1149: Doctest examples won't compile for private functions.The examples in the rustdoc comments reference
read_leading_triviaandread_trailing_triviadirectly, but since these are private functions, the doctests won't run. Either add# ignoreto the code blocks or convert them to unit tests if you want them validated.Option 1: Mark examples as ignored
/// # Examples -/// ``` +/// ```ignore /// assert_eq!(read_leading_trivia("\n\tconsole.log('Hi');"), "\n\t");Option 2: Add unit tests instead
#[cfg(test)] mod tests { use super::*; #[test] fn test_read_leading_trivia() { assert_eq!(read_leading_trivia("\n\tconsole.log('Hi');"), "\n\t"); assert_eq!(read_leading_trivia("console.log('Hi');"), ""); assert_eq!(read_leading_trivia(" "), " "); } #[test] fn test_read_trailing_trivia() { assert_eq!(read_trailing_trivia("console.log('Hi');\n"), "\n"); assert_eq!(read_trailing_trivia("console.log('Hi');"), ""); assert_eq!(read_trailing_trivia(" "), ""); } }As per coding guidelines, doc tests should compile and pass.
Also applies to: 1177-1182
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
ematipico
force-pushed
the
feat/consistent-foramtting
branch
from
975ea3e to
e56de71
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@crates/biome_cli/src/commands/check.rs`:
- Around line 79-81: The not_requested_features() implementation builds an empty
set then removes search (no-op), producing the wrong feature set; update the
builder in not_requested_features() to explicitly include the search feature
(use FeaturesBuilder::new().with_search().build()) so the function returns the
"only search" set rather than an empty set—follow the same pattern used in
format.rs (with_all().without_formatter()) to express “all except X” correctly.
In `@crates/biome_lsp/src/handlers/analysis.rs`:
- Around line 83-84: The code_actions path currently sets not_requested_features
to FeaturesBuilder::new().with_search().build() but forgets to mark the
formatter as not requested; update the not_requested_features in the
code_actions call to include formatter (i.e. use
FeaturesBuilder::new().with_search().with_formatter().build()) so the formatter
is treated as not requested (keep fix_all as-is which explicitly requests
formatter).
In `@crates/biome_service/src/workspace.rs`:
- Around line 349-354: The logic is inconsistent: is_ignored() now treats
SupportKind::is_not_requested() as ignored but is_not_processed() still treats
it as processed; update the predicate in is_not_processed (and the similar
method around lines 377-388) to include support_kind.is_not_requested() where
you currently check is_ignored() or is_not_requested() so NotRequested is
treated as "not processed"/ignored consistently; adjust the iterator conditions
in the is_not_processed implementation(s) to include
support_kind.is_not_requested() alongside is_ignored() and/or is_not_requested()
as appropriate.
2 participants
Summary
This PR fixes two bugs.
Fixes #7912, where embedded formatting was incorrectly applied. I left a comment explaining the change.
The second bug is the CLI, which didn't apply formatting to the code actions when using
Workspace::fix_all. This works as expected in the LSP. As you can see, there are many changes because... well, we never caught it 😅Test Plan
Added a new test for the first bug. Updated the snapshots and tests for the second bug.
Docs
N/A