Conversation
🦋 Changeset detectedLatest commit: 53d55a0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 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 |
WalkthroughA changeset was added to bump @biomejs/biome as a patch. In crates/biome_fs/src/fs.rs, noisy error logging was removed from file-read flows so errors now propagate via FileSystemDiagnostic; auto_search_files_with_predicate was refactored to a match-driven loop with clearer control flow and a concise diagnostic "Couldn't find the configuration file at {}." No public APIs or exported entities were changed. The changes target issue #7788 by preventing spurious error logs when probing for configuration files. Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/biome_fs/src/fs.rs (1)
184-187: Update the documentation to reflect removed logging.The documentation states the method "logs an error message", but the error logging has been removed. Please update the doc comment to reflect the current behaviour.
Apply this diff:
/// Reads the content of a file specified by `file_path`. /// /// This method attempts to open and read the entire content of a file at the given path. /// /// ## Errors - /// This method logs an error message and returns a `FileSystemDiagnostic` error in two scenarios: + /// This method returns a `FileSystemDiagnostic` error in two scenarios: /// - If the file cannot be opened, possibly due to incorrect path or permission issues. /// - If the file is opened but its content cannot be read, potentially due to the file being damaged.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/gentle-ears-itch.md(1 hunks)crates/biome_fs/src/fs.rs(1 hunks)package.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_fs/src/fs.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs: Format Rust files before committing (e.g., viajust fwhich formats Rust)
Document rules, assists, and options with inline rustdoc in source
Files:
crates/biome_fs/src/fs.rs
.changeset/*.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
.changeset/*.md: In changesets, only use #### or ##### headers; other header levels are not allowed
Changesets should cover user-facing changes only; internal changes do not need changesets
Use past tense for what you did and present tense for current Biome behavior in changesets
When fixing a bug in a changeset, start with an issue link (e.g., “Fixed #1234: …”)
When referencing a rule or assist in a changeset, include a link to its page on the website
Include code blocks in changesets when applicable to illustrate changes
End every sentence in a changeset with a period
Files:
.changeset/gentle-ears-itch.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Check JS Files
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Test Node.js API
- GitHub Check: Documentation
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Check Dependencies
- GitHub Check: autofix
🔇 Additional comments (2)
package.json (1)
17-17: Verify the pnpm version bump is intentional.The PR objective is to remove error logging, but this line updates the pnpm engine version. Is this change intentional, or was it accidentally included?
crates/biome_fs/src/fs.rs (1)
194-207: LGTM! Error logging removal fixes the issue.The changes correctly remove automatic error logging whilst preserving the full diagnostic information for callers. This resolves issue #7788 where optional config file searches would spam error logs even when the user requested
--log-level=error.
fadd00e to
a8931d6
Compare
dyc3
left a comment
There was a problem hiding this comment.
I think it would be more useful to just change the log level for this, and warn when we don't find any config. No strong feelings though
Agreed, a warn for this kind of "error" would be more appropriate. And throw an actual error log if no config is found at all. In this case, with the config, it might be confusing as it does not mention it will try other file extensions. As if this error was the no config was found error, while it was only about the json variant |
|
In this circumstance, the log should be implemented by the caller, because in case of a parent resolution the error is handled differently (config file and ignore file), but in case of simple files, there should be an error. That's why we should remove the logging here. I'll update the PR to implement the logging where we load the configuration file. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/biome_fs/src/fs.rs (1)
188-195: Update the outdated documentation.The doc comment still claims this method "logs an error message", but error logging was removed in lines 202–215.
Apply this diff to update the documentation:
/// Reads the content of a file specified by `file_path`. /// /// This method attempts to open and read the entire content of a file at the given path. /// /// ## Errors - /// This method logs an error message and returns a `FileSystemDiagnostic` error in two scenarios: + /// This method returns a `FileSystemDiagnostic` error in two scenarios: /// - If the file cannot be opened, possibly due to incorrect path or permission issues. /// - If the file is opened but its content cannot be read, potentially due to the file being damaged.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/biome_fs/src/fs.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_fs/src/fs.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs: Format Rust files before committing (e.g., viajust fwhich formats Rust)
Document rules, assists, and options with inline rustdoc in source
Files:
crates/biome_fs/src/fs.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Documentation
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: End-to-end tests
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Check Dependencies
- GitHub Check: Test Node.js API
- GitHub Check: autofix
🔇 Additional comments (2)
crates/biome_fs/src/fs.rs (2)
14-14: Import change aligns with removing error logs.Removing the
errorimport is consistent with the PR's goal of moving logging responsibility to the caller.
202-216: Error log removal achieves the stated goal.Returning
FileSystemDiagnosticwithout logging allows callers to decide the appropriate log level (e.g., treating missing config files as info rather than error during auto-search).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/biome_fs/src/fs.rs (3)
160-165: Log the file path, not the directory (and tighten the copy).Message claims “the file at the following path” but logs the directory. Use the actual file path and shorter wording.
- if is_searching_in_parent_dir { - info!( - "Biome auto discovered the file at the following path that isn't in the working directory:\n{:?}", - current_search_dir - ); - } + if is_searching_in_parent_dir { + info!( + "Biome auto-discovered a configuration file in a parent directory: {}", + file_path.as_str() + ); + }
172-176: Noisy info logs for expected misses; let the caller decide.Logging “Couldn't find the configuration file …” on every miss will spam on upward traversal. Given the PR goal (shift logging to the caller), drop this log or downgrade to trace.
- Err(_) => { - info!( - "Couldn't find the configuration file at {}.", - file_path.as_str() - ); - } + Err(_) => { + // Silently continue; caller will handle final reporting. + }If you remove both logs, also delete
use tracing::info;at Line 14.
196-201: Docstring no longer matches behaviour.This function doesn’t log errors anymore; it only returns a diagnostic. Update the docs to avoid confusion.
- /// ## Errors - /// This method logs an error message and returns a `FileSystemDiagnostic` error in two scenarios: - /// - If the file cannot be opened, possibly due to incorrect path or permission issues. - /// - If the file is opened but its content cannot be read, potentially due to the file being damaged. + /// ## Errors + /// Returns a `FileSystemDiagnostic` when: + /// - The file cannot be opened (e.g. incorrect path or permissions). + /// - The file is opened but its content cannot be read (e.g. I/O errors or corruption).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/biome_fs/src/fs.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_fs/src/fs.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs: Format Rust files before committing (e.g., viajust fwhich formats Rust)
Document rules, assists, and options with inline rustdoc in source
Files:
crates/biome_fs/src/fs.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Documentation
- GitHub Check: End-to-end tests
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Check Dependencies
- GitHub Check: autofix
- GitHub Check: Test Node.js API
Summary
Closes #7788
To be honest, I am not sure if that's what OP was complaining about because there seems to be some problems with "lost in translation". Regardless, those error logs are useless there.
Test Plan
N/A
Docs
N/A