-
Notifications
You must be signed in to change notification settings - Fork 927
Add concrete FlightRecorderDiskSource #6281
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| ) : BaseDiskSource(sharedPreferences = sharedPreferences), | ||
| SettingsDiskSource { | ||
| SettingsDiskSource, | ||
| FlightRecorderDiskSource by flightRecorderDiskSource { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a little torn with this.
On one hand, this centralizes a decent bit of logic but it is a bit cumbersome in the these places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Claude. The trade-off here is worth it. 🚀
|
Claude finished @david-livefront's task in 4m 2s —— View job Code Review SummaryStatus: ✅ Approved with minor suggestion This PR successfully extracts flight recorder disk operations into a dedicated OverviewChange Type: Refactoring Key Changes
Architecture Assessment✅ Separation of concerns - Flight recorder logic properly isolated Findings🎨 Suggested (1)
Review CompleteThis is a clean refactoring that improves maintainability without introducing functional changes. The delegation pattern is well-applied, dependencies are correctly wired, and test coverage is maintained. The single finding is a minor typo that doesn't affect functionality. 🤖 Generated with Claude Code Co-Authored-By: Claude Opus 4.5 noreply@anthropic.com |
ca4f944 to
48d8493
Compare
48d8493 to
705cbff
Compare
|
Great job! No new security vulnerabilities introduced in this pull request |
d0a1aca to
2394949
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6281 +/- ##
==========================================
+ Coverage 85.45% 85.52% +0.06%
==========================================
Files 776 763 -13
Lines 54949 54621 -328
Branches 7854 7852 -2
==========================================
- Hits 46957 46712 -245
+ Misses 5264 5181 -83
Partials 2728 2728 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2394949 to
83f6dc6
Compare
...test/kotlin/com/x8bit/bitwarden/data/platform/datasource/disk/util/FakeSettingsDiskSource.kt
Outdated
Show resolved
Hide resolved
83f6dc6 to
4adf00a
Compare
data/src/test/kotlin/com/bitwarden/data/datasource/disk/FlightRecorderDiskSourceTest.kt
Outdated
Show resolved
Hide resolved
4adf00a to
7a17b9f
Compare
|
Thanks @SaintPatrck |

🎟️ Tracking
PM-29912
📔 Objective
This PR Moves as much logic as possible into a central location and exposes it via the app-specific
SettingsRepositorys.⏰ Reminders before review
🦮 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