kotlin: add library request interfaces#240
Conversation
goaway
left a comment
There was a problem hiding this comment.
Overall this is great. Fully-agree Builder pattern is the way to go here. I left links to some comments on @rebello95's other PR, since they're the same and we'll want consistency.
Signed-off-by: Alan Chiu <achiu@lyft.com>
Signed-off-by: Alan Chiu <achiu@lyft.com>
Signed-off-by: Alan Chiu <achiu@lyft.com>
Signed-off-by: Alan Chiu <achiu@lyft.com>
Signed-off-by: Alan Chiu <achiu@lyft.com>
f988b2b to
021c353
Compare
Signed-off-by: Alan Chiu <achiu@lyft.com>
| .addRetryPolicy(retryPolicy) | ||
| .build() | ||
|
|
||
| assertThat(request.retryPolicy).isEqualTo(RetryPolicy(23, listOf(RetryRule.FIVE_XX, RetryRule.CONNECT_FAILURE), 1234)) |
There was a problem hiding this comment.
Why not compare directly to retryPolicy?
There was a problem hiding this comment.
It's easier to see the values which we are expecting on an assert line
| ) | ||
|
|
||
| kt_jvm_library( | ||
| name = "envoy_interfaces_lib", |
There was a problem hiding this comment.
Shouldn't this be part of envoy_lib?
There was a problem hiding this comment.
It'll pull in all the dependencies with the JNI, Android, etc. I wanted to keep this more vanilla kotlin for now because I hope to isolate most of the classes away from Android and Envoy. The rationale is because I want to keep the tests lighter weight and we can more easily make decisions independent from the other layers.
We can also revisit the organization and responsibility boundaries down the line
|
thanks for the review @rebello95 @goaway |
#119
Adding a primitive
RequestandRequestBuilderwith basic features:Tests are basic setting a builder and ensuring the result is expected. Additionally, the request build transform creates an equivalent object when built back.
Signed-off-by: Alan Chiu achiu@lyft.com
For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md
Description: kotlin: add library request interfaces
Risk Level: low
Testing: unit
Docs Changes: n/a
Release Notes: n/a
[Optional Fixes #Issue]
[Optional Deprecated:]