Skip to content

Conversation

Hinton
Copy link
Member

@Hinton Hinton commented Sep 16, 2025

🎟️ Tracking

📔 Objective

  • Enable dylint in CI and pre-commit.
  • Fix remaining violations.
  • Tweak linter to avoid catching Error variants in non error enums.

⏰ 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

Copy link
Contributor

github-actions bot commented Sep 16, 2025

Logo
Checkmarx One – Scan Summary & Details663bb3e6-9482-465c-99d3-d98cf73c28e1

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

Copy link

codecov bot commented Sep 16, 2025

Codecov Report

❌ Patch coverage is 45.83333% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.26%. Comparing base (31b18f0) to head (a57ffed).
⚠️ Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
crates/bitwarden-fido/src/lib.rs 63.63% 4 Missing ⚠️
crates/bitwarden-crypto/src/rsa.rs 0.00% 2 Missing ⚠️
crates/bitwarden-crypto/src/store/context.rs 0.00% 2 Missing ⚠️
crates/bitwarden-uniffi/src/error.rs 0.00% 2 Missing ⚠️
...rates/bitwarden-crypto/src/enc_string/symmetric.rs 0.00% 1 Missing ⚠️
crates/bitwarden-crypto/src/keys/master_key.rs 0.00% 1 Missing ⚠️
crates/bitwarden-uniffi/src/lib.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #458      +/-   ##
==========================================
+ Coverage   76.92%   78.26%   +1.34%     
==========================================
  Files         270      275       +5     
  Lines       25582    27444    +1862     
==========================================
+ Hits        19678    21479    +1801     
- Misses       5904     5965      +61     

☔ 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.

@Hinton Hinton marked this pull request as ready for review September 16, 2025 13:50
@Hinton Hinton requested review from a team as code owners September 16, 2025 13:50
coroiu
coroiu previously approved these changes Sep 16, 2025
run: cargo install cargo-dylint dylint-link --version 4.1.0 --locked

- name: Cargo dylint
run: cargo dylint --all
Copy link
Member

Choose a reason for hiding this comment

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

Should this instead be like this so it tests feature flagged code and doctests/examples?

Suggested change
run: cargo dylint --all
run: cargo dylint --all -- --all-features --all-targets

Same thing on the readme and lint staged file too

Copy link
Member Author

Choose a reason for hiding this comment

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

True, I removed clippy since dylint includes clippy.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that's true, for me dylint only seems to be checking it's own lints. It's not warning about other clippy lints.

For example, add this in any file:

        let _ = 'a'..'z';

Clippy will warn against it but dylint will not

Copy link
Member Author

Choose a reason for hiding this comment

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

You're correct. Re-added clippy.

mzieniukbw
mzieniukbw previously approved these changes Sep 17, 2025
@Hinton Hinton dismissed stale reviews from mzieniukbw and coroiu via 55db2aa September 18, 2025 11:37
coroiu
coroiu previously approved these changes Sep 18, 2025
mzieniukbw
mzieniukbw previously approved these changes Sep 18, 2025
gbubemismith
gbubemismith previously approved these changes Sep 18, 2025
@Hinton Hinton dismissed stale reviews from mzieniukbw and coroiu via a57ffed September 18, 2025 15:41
Copy link

@Hinton Hinton requested a review from coroiu September 18, 2025 15:50
@Hinton Hinton merged commit e30f1ea into main Sep 19, 2025
51 checks passed
@Hinton Hinton deleted the arch/error-lint-fix branch September 19, 2025 08:14
bw-ghapp bot pushed a commit to bitwarden/sdk-swift that referenced this pull request Sep 19, 2025
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.

5 participants