-
Notifications
You must be signed in to change notification settings - Fork 11
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
Cleanup URLConnection wrappering code #29
Conversation
Current dependencies on/for this PR: This comment was auto-generated by Graphite. |
b7ef2e8
to
fd3baaa
Compare
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.
Left a few comments inline. I'm slightly nervous about making a change in this area as it's pretty complex - what is the general test coverage looking like these days? Do we need to perform any manual testing to sanity check that requests show up in the dash?
...rc/main/java/io/embrace/android/embracesdk/okhttp3/EmbraceOkHttp3ApplicationInterceptor.java
Show resolved
Hide resolved
...c/main/java/io/embrace/android/embracesdk/internal/network/http/EmbraceHttpPathOverride.java
Show resolved
Hide resolved
...java/io/embrace/android/embracesdk/internal/network/http/EmbraceUrlStreamHandlerFactory.java
Show resolved
Hide resolved
Right. I should have been more clear - I didn't actually change anything, just fixed some IDE suggested warnings, renamed the classes, and moved them. It's all broken down in the separate commits for clarity. I wasn't actually going to make material changes until I have more tests. Right now, I have decent coverage of the wrapper (newly renamed I fact, I don't plan to make significant changes - I just don't think it's worth it given the risks and gains. But we do want test coverage so I plan to add that, but I wanted to do this earlier so the tests would make more sense initially. |
d888d19
to
37750d0
Compare
fd3baaa
to
cbbc0aa
Compare
37750d0
to
bb8e459
Compare
6a5cc59
to
ccef18c
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #29 +/- ##
=======================================
Coverage 72.66% 72.66%
=======================================
Files 305 305
Lines 9986 9992 +6
Branches 1440 1440
=======================================
+ Hits 7256 7261 +5
- Misses 2161 2162 +1
Partials 569 569
|
ccef18c
to
f69c2bf
Compare
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.
LGTM
Goal
Clean up URLConnection wrapping code by putting them in the right internal package and giving them names that are more appropriate. No material changes to logic have been made - check out the individual commits for details.