Skip to content

Conversation

@bartlomieju
Copy link
Member

No description provided.

@coderabbitai
Copy link

coderabbitai bot commented Nov 28, 2025

Walkthrough

The change adds a normalization layer to package entry processing in the package manager module. Entries without an explicit npm: or jsr: prefix are automatically prefixed with npm: before parsing. Entries that already have a prefix are passed through unchanged. Error messages continue to reference the original unprefixed entry text, maintaining clarity in error reporting while ensuring consistent handling by the parser.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Description check ❓ Inconclusive No description was provided by the author, making it impossible to assess whether it relates to the changeset. Add a pull request description explaining the motivation and impact of removing the npm: prefix requirement.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: allowing package installation without requiring the npm: prefix.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
cli/tools/pm/mod.rs (1)

458-463: Normalization logic looks correct.

The Cow usage efficiently avoids allocation when a prefix already exists. Note that this normalization means the default_registry parameter (line 466) is now effectively ignored for unprefixed entries, since they'll always get the npm: prefix here. This appears intentional based on the PR objective.

Consider adding a test case that explicitly covers this normalization behavior, e.g., verifying that "foo" (without prefix) gets treated as "npm:foo":

#[test]
fn test_unprefixed_defaults_to_npm() {
  // Test that unprefixed package names are treated as npm packages
  // This tests the normalization that happens in the add() function
  // before calling parse()
}
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4c228b4 and f055a1e.

📒 Files selected for processing (1)
  • cli/tools/pm/mod.rs (2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
cli/tools/**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

CLI tools should be implemented in cli/tools/<tool> or cli/tools/<tool>/mod.rs

Files:

  • cli/tools/pm/mod.rs
**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or use lldb directly
Use eprintln!() or dbg!() macros for debug prints in Rust code

Files:

  • cli/tools/pm/mod.rs
🧬 Code graph analysis (1)
cli/tools/pm/mod.rs (1)
libs/package_json/lib.rs (1)
  • parse (97-175)
⏰ 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: test debug linux-aarch64
  • GitHub Check: test debug linux-x86_64
  • GitHub Check: test debug macos-aarch64
  • GitHub Check: test debug windows-x86_64
  • GitHub Check: test debug macos-x86_64
  • GitHub Check: lint debug macos-x86_64
  • GitHub Check: lint debug windows-x86_64
  • GitHub Check: lint debug linux-x86_64
  • GitHub Check: build libs
🔇 Additional comments (2)
cli/tools/pm/mod.rs (2)

3-3: LGTM!

Import is necessary for the Cow usage below.


464-468: LGTM!

Correctly uses the normalized text for parsing while preserving the original entry_text in error messages for better user experience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant