Convert HttpClient to Kotlin #1215
Conversation
| } catch (ignore: MalformedURLException) { | ||
| } catch (ignore: URISyntaxException) { | ||
| } |
There was a problem hiding this comment.
Are we able to add a comment or something in there blocks to indicate why we ignore these? To me they seem like something we wouldn't want to ignore, but I see we were also doing this previously.
There was a problem hiding this comment.
I'm not too sure why we're catching these 😅 but from a quick look, HttpRequest.getURL() can throw these exceptions and it feels like we don't want to crash due to those.
I think a lot of the networking code can be refactored to remove the ambiguity, but I wanted to keep the actual code changes to a minimum other than the java to kotlin conversion.
There was a problem hiding this comment.
That sounds good, yeah it's not blocking was more for my general understanding.
| } | ||
| } | ||
|
|
||
| private fun getNumRetriesSoFar(url: URL): Int { |
There was a problem hiding this comment.
Not sure if getCurrentRetryCount would be a better name for this. What do you think?
There was a problem hiding this comment.
That makes sense to me! Although I feel the rename should be done in a separate PR since we wouldn't have git history of the change since we're converting from java to kotlin in this PR.
There was a problem hiding this comment.
We can do that in a separate PR if that will make the history easier. Are you able to make a ticket for that?
There was a problem hiding this comment.
Yep! I'll create a ticket for tracking the rename.
| } catch (ignore: MalformedURLException) { | ||
| } catch (ignore: URISyntaxException) { | ||
| } |
There was a problem hiding this comment.
Same question here as above, are we able to add a comment in these blocks for why we ignore these?
There was a problem hiding this comment.
It's calling the same HttpRequest.getUrl() function which can throw both of those exceptions.
SharedUtils/src/main/java/com/braintreepayments/api/sharedutils/SynchronousHttpClient.kt
Show resolved
Hide resolved
SharedUtils/src/main/java/com/braintreepayments/api/sharedutils/SynchronousHttpClient.kt
Outdated
Show resolved
Hide resolved
| @@ -137,36 +115,6 @@ public void request_whenConnectionIsHttps_setsSSLSocketFactory() throws Exceptio | |||
| verify(connection).setSSLSocketFactory(sslSocketFactory); | |||
| } | |||
|
|
|||
| @Test | |||
| public void request_whenConnectionIsHttps_andSSLSocketFactoryIsNull_throwsSSLException() | |||
There was a problem hiding this comment.
Are these tests no longer relevant with the updated logic?
There was a problem hiding this comment.
Yep, these tests are no longer relevant since SSLSocketFactory is now non-null in Kotlin.
…s/SynchronousHttpClient.kt Co-authored-by: Jax DesMarais-Leder <[email protected]>
Summary of changes
HttpClientandSynchronusHttpClientto KotlinChecklist
Authors