Skip to content

Conversation

@neuronull
Copy link
Contributor

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-28762

📔 Objective

Now that the SDK is using tracing instead of log, add the same clippy lint to disallow the log crate's macros as we have in clients repo.

🚨 Breaking Changes

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation
    team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed
    issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

@neuronull neuronull self-assigned this Nov 24, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Nov 24, 2025

🔍 SDK Breaking Change Detection Results

SDK Version: neuronull/clippy-disallow-log-crate-macros (081dcd4)
Completed: 2025-11-24 15:17:56 UTC
Total Time: 218s

Client Status Details
typescript ❌ Breaking changes detected TypeScript compilation failed with new SDK version - View Details

Breaking change detection completed. View SDK workflow

@github-actions
Copy link
Contributor

Logo
Checkmarx One – Scan Summary & Details793feae2-6ee8-4014-b5ca-fe2739e1f160

Great job! No new security vulnerabilities introduced in this pull request

@codecov
Copy link

codecov bot commented Nov 24, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.51%. Comparing base (d05b2ae) to head (081dcd4).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #581   +/-   ##
=======================================
  Coverage   79.51%   79.51%           
=======================================
  Files         302      302           
  Lines       32311    32311           
=======================================
  Hits        25691    25691           
  Misses       6620     6620           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@neuronull neuronull marked this pull request as ready for review November 24, 2025 15:28
@neuronull neuronull requested a review from a team as a code owner November 24, 2025 15:28
@claude
Copy link

claude bot commented Nov 24, 2025

Claude finished @neuronull's task —— View job


Code Review: Clippy disallow log crate macros

This PR adds clippy lints to enforce the migration from log to tracing by disallowing all log crate macros.


Summary of Changes

Single commit adding clippy configuration to disallow log crate macros:

  • Cargo.toml: Enables disallowed-macros lint at workspace level (line 112)
  • clippy.toml: Defines disallowed macro list covering all five log macros with helpful replacement guidance (lines 12-18)

Critical Issues Found

Finding 1: 💭 Breaking change detection shows TypeScript compilation failure, but this appears unrelated to the PR changes since no runtime behavior or API surface is modified - only developer-facing lint rules were added.

Context

The breaking change detection reported: "TypeScript compilation failed with new SDK version" (details). However, this PR only modifies clippy configuration files that affect compile-time checks for Rust developers. No code changes were made that would affect:

  • Public APIs
  • FFI bindings
  • Generated TypeScript types
  • Runtime behavior

The TypeScript failure is likely either:

  1. A pre-existing issue in the clients repo
  2. An unrelated infrastructure/environment issue
  3. A false positive from the detection mechanism

Recommendation: Verify the clients workflow failure is unrelated before merging, or document why it's a false positive.


Suggested Improvements

None. The implementation is correct and complete.


Good Practices Observed

  • Clean configuration following established patterns from the clients repo
  • Comprehensive coverage of all five log macros
  • Clear reason and replacement guidance for developers encountering the lint
  • Workspace-level enforcement ensures consistency across all crates

PR Metadata Quality

Finding 2: 💭 PR description could be enhanced with verification details.

Suggested improvements

The PR description is good overall with clear objective and Jira reference. Consider adding:

Test Plan section:

## Test plan

- Verified no existing `log::` macro usage in codebase via `rg 'log::(trace|debug|info|warn|error)!'`
- Confirmed clippy configuration is valid
- All CI checks passed (security scan, code coverage)

This would document how the changes were validated and provide confidence that the migration is complete.


Action Items

  1. ⚠️ Investigate the TypeScript breaking change detection failure to confirm it's unrelated to this PR
  2. Optional: Add test plan details to PR description documenting verification steps

@neuronull neuronull merged commit f75a58c into main Nov 24, 2025
63 of 64 checks passed
@neuronull neuronull deleted the neuronull/clippy-disallow-log-crate-macros branch November 24, 2025 16:21
bw-ghapp bot pushed a commit to bitwarden/sdk-swift that referenced this pull request Nov 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants