Skip to content
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

Create OS-specific implementations of the ActivityLoadEventEmitter #1683

Merged
merged 1 commit into from
Nov 29, 2024

Conversation

bidetofevil
Copy link
Collaborator

@bidetofevil bidetofevil commented Nov 15, 2024

Goal

Split out OS version specific handling of UiLoad events into separate classes. Tried to use a delegate but couldn't get it to work properly, so I just created two implementation with a shared object to handle common functionality. Good ol' situation where composition vs inheritance has no obvious winner, so I just picked one.

Testing

Tweaked existing tests to make sure things work

Copy link

codecov bot commented Nov 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.36%. Comparing base (faaff58) to head (f37bc95).
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1683      +/-   ##
==========================================
+ Coverage   85.34%   85.36%   +0.02%     
==========================================
  Files         463      463              
  Lines       10698    10706       +8     
  Branches     1600     1600              
==========================================
+ Hits         9130     9139       +9     
  Misses        852      852              
+ Partials      716      715       -1     
Files with missing lines Coverage Δ
.../embracesdk/internal/capture/activity/UiLoadExt.kt 100.00% <100.00%> (ø)
...internal/injection/DataCaptureServiceModuleImpl.kt 95.23% <100.00%> (ø)

... and 1 file with indirect coverage changes

@bidetofevil bidetofevil marked this pull request as ready for review November 15, 2024 15:51
@bidetofevil bidetofevil requested a review from a team as a code owner November 15, 2024 15:51
@bidetofevil bidetofevil requested review from fractalwrench and priettt and removed request for a team November 15, 2024 15:51
Copy link
Contributor

@fractalwrench fractalwrench left a comment

Choose a reason for hiding this comment

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

LGTM

@bidetofevil bidetofevil force-pushed the hho/ui-load-naming branch 2 times, most recently from 79870f7 to 5abae2a Compare November 29, 2024 19:28
@bidetofevil bidetofevil changed the base branch from hho/ui-load-naming to graphite-base/1683 November 29, 2024 19:40
@bidetofevil bidetofevil changed the base branch from graphite-base/1683 to main November 29, 2024 19:41
Copy link
Contributor

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

PackageVersionScoreDetails

Scanned Files

@bidetofevil bidetofevil merged commit f69b5eb into main Nov 29, 2024
8 checks passed
Copy link
Collaborator Author

Merge activity

  • Nov 29, 2:52 PM EST: A user merged this pull request with Graphite.

@bidetofevil bidetofevil deleted the hho/split-load-event-emitters branch November 29, 2024 19:52
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.

2 participants