refactor: remove #[allow(dead_code)] directives and fix warnings#89
Conversation
- Remove all #[allow(dead_code)] annotations per project rules - Refactor main.rs to use library imports instead of mod declarations - Mark test-only code with #[cfg(test)] instead of allow attributes - Add tests for cache trait methods (contains, remove, clear) - Remove unused from_env() method from EnvTokenProvider - Clean up npmrc and cargo_credentials modules All 275 tests pass, clippy clean with -D warnings.
📝 WalkthroughWalkthroughThis PR moves several authentication and credential-parsing APIs to test-only scope (#[cfg(test)]), removes disk I/O for .npmrc and Cargo credentials parsing in favor of in-memory parsing helpers used by tests, adds unit tests, and updates crate import paths to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dependi-lsp/src/auth/mod.rs (1)
102-115: Outdated doc example references removedfrom_envmethod.The example on lines 109-110 references
EnvTokenProvider::from_env("GITHUB_TOKEN"), but according to the AI summary, thefrom_env()method was removed in this PR. While theignoreattribute prevents doc test failures, the example is now misleading.📝 Suggested fix
/// ```ignore /// use dependi_lsp::auth::{TokenProviderManager, EnvTokenProvider}; /// use std::sync::Arc; /// /// let manager = TokenProviderManager::new(); /// /// // Register provider for GitHub npm -/// if let Some(provider) = EnvTokenProvider::from_env("GITHUB_TOKEN") { -/// manager.register("https://npm.pkg.github.com", Arc::new(provider)).await; +/// if let Ok(token) = std::env::var("GITHUB_TOKEN") { +/// let provider = EnvTokenProvider::new(token); +/// manager.register("https://npm.pkg.github.com".to_string(), Arc::new(provider)).await; /// } /// /// // Get headers for a request /// let headers = manager.get_auth_headers("https://npm.pkg.github.com/@org/package").await; /// ```
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
dependi-lsp/src/auth/cargo_credentials.rsdependi-lsp/src/auth/mod.rsdependi-lsp/src/auth/npmrc.rsdependi-lsp/src/cache/mod.rsdependi-lsp/src/main.rs
🧰 Additional context used
🧬 Code graph analysis (1)
dependi-lsp/src/cache/mod.rs (1)
dependi-lsp/src/cache/sqlite.rs (2)
create_test_version_info(349-364)get(206-227)
⏰ 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). (4)
- GitHub Check: Code coverage
- GitHub Check: test
- GitHub Check: security-audit
- GitHub Check: Analyze (rust)
🔇 Additional comments (8)
dependi-lsp/src/main.rs (1)
8-8: LGTM! Import paths correctly updated for binary-to-library crate imports.Changing from
crate::todependi_lsp::is the correct approach for a binary crate (main.rs) importing from its sibling library crate. This eliminates the duplicate module compilation issue mentioned in the PR summary.Also applies to: 83-88
dependi-lsp/src/cache/mod.rs (1)
449-483: LGTM! Well-structured tests for cache trait methods.The new tests properly validate
contains,remove, andclearoperations through trait objects (&dyn ReadCache,&dyn WriteCache), ensuring the trait implementations work correctly via dynamic dispatch. This addresses the PR goal of adding tests for previously#[allow(dead_code)]-annotated methods.dependi-lsp/src/auth/cargo_credentials.rs (2)
10-42: LGTM! Parsing utilities correctly scoped to test-only.The
#[cfg(test)]annotations appropriately gate the parsing logic that was previously marked with#[allow(dead_code)]. The documentation note on lines 5-8 clearly explains that I/O integration will be added when this is wired into the main auth flow.
44-110: Good test coverage for credential parsing edge cases.Tests comprehensively cover: valid credentials, empty content, missing registries section, registry without token, and mixed registries with/without tokens. This validates the parsing logic before I/O integration.
dependi-lsp/src/auth/npmrc.rs (2)
114-148: Appropriate use ofserial_testand documented unsafe blocks for env var tests.The
#[serial]attribute andunsafeblocks with SAFETY comments correctly document whystd::env::set_var/remove_varrequire unsafe in Rust 2024 edition (they're not thread-safe). The cleanup in each test prevents test pollution.
10-89: LGTM! Parsing utilities correctly scoped to test-only with proper env var handling.The parsing functions are appropriately gated with
#[cfg(test)]. Theresolve_env_varfunction correctly handles both${VAR}and$VARsyntaxes, and theextract_auth_tokenfunction properly handles both direct and registry-scoped token formats. The project's Rust edition is set to 2024, which fully supports the let-chain syntax used throughout these functions.dependi-lsp/src/auth/mod.rs (2)
81-89: LGTM!NoAuthProvidercorrectly scoped to test-only.Since
NoAuthProvideris only used in test scenarios, gating it with#[cfg(test)]is the appropriate replacement for#[allow(dead_code)].
162-163: No changes needed — test-gating is correct.Verification confirms that
get_auth_headersandhas_provideronTokenProviderManagerare only called from test code (lines 274, 288). Production code only callsprovider_count(), which remains available. The#[cfg(test)]gates are appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
dependi-lsp/src/auth/mod.rs (2)
102-116: Documentation example references test-only API.The example at line 115 calls
manager.get_auth_headers(), but this method is now restricted to tests (marked#[cfg(test)]at line 163). This creates a misleading example for users, as the shown API is not available in production builds.Consider either:
- Updating the example to reflect actual production usage patterns
- Moving this example to test documentation
- Removing the example if
TokenProviderManageris now internal/test-only
163-194: Remove#[cfg(test)]or reconsider the public API design.
get_auth_headers()is marked test-only whileregister()remains public, creating an inconsistent API. Currently, neither method is used in production code—TokenProviderManageris only instantiated and used to callprovider_count()for logging.If this infrastructure is incomplete, either:
- Remove public visibility from
register()and mark the entire struct#[cfg(test)], or- Remove
#[cfg(test)]fromget_auth_headers()to allow future production integration without requiring code restructuring.
🧹 Nitpick comments (1)
dependi-lsp/src/auth/mod.rs (1)
197-201: Consider the API design implications of test-only accessors.While
register()remains public,has_provider()is now test-only. This creates an asymmetric API where callers can add providers but cannot query them. IfTokenProviderManageris meant for production use, consider whether diagnostic methods like this should remain public for observability.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dependi-lsp/src/auth/mod.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). (4)
- GitHub Check: Code coverage
- GitHub Check: test
- GitHub Check: security-audit
- GitHub Check: Analyze (rust)
🔇 Additional comments (2)
dependi-lsp/src/auth/mod.rs (2)
226-347: Comprehensive test coverage.The test suite covers the authentication functionality well, including edge cases like UTF-8 safety in token redaction, HTTPS enforcement, and longest-prefix matching for provider selection.
81-89: The#[cfg(test)]annotation onNoAuthProvideris appropriate. Verification confirms thatNoAuthProvideris not referenced in any production code outside of test modules, so restricting it to test-only is correct.
1 participant
Summary
#[allow(dead_code)]annotations per project rules (CLAUDE.md forbids bypassing errors with#[allow(...)])main.rsto use library imports instead ofmoddeclarations, eliminating duplicate module compilation#[cfg(test)]instead of allow attributescontains,remove,clear)from_env()method fromEnvTokenProvidernpmrcandcargo_credentialsmodules by removing untested file I/O wrappersTest plan
cargo clippy --all-targets -- -D warningspasses with no warningscargo test --lib- all 275 tests passcargo fmt --all -- --checkpassesSummary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.