Introduce ExperimentRestClient and basic domain models#237
Conversation
`okhttp` and `moshi` are used by possible consumers. Code generation is faster and safer in the runtime, hence `moshi-codegen`
So we can use a double in tests
Usually, using `data class` in a public contract is not the best idea, but I think it's fine with this internal-only library. We don't have to be *that* strict with backward compatibility
So it can be replaced with a double in tests
To match to the current project's convention
It doesn't have to be exposed to library consumer
ParaskP7
left a comment
ParaskP7
left a comment
There was a problem hiding this comment.
👋 @wzieba !
I have reviewed this PR as per the instructions, everything LGTM, good job! 🌟
FYI: I also traversed the FluxC related repo and its ExperimentRestClient + AssignmentsModel implementation and everything seems to be mapped 1-to-1 with your Moshi solution here, well done. 🥇
I decided to not introduce a feature branch to keep things simpler: newly introduced classes are not visible to consumers (except domain models, which are useless on its own) so it shouldn't cause confusion.
👍
At this moment, testing is not required or possible due to lack of an entry point. The testing is covered with unit tests, and in next PRs I'll prepare some test scenarios.
👍
I have left one question (❓), a suggestion (💡) and some minor (🔍) comments for you to consider. I am going to approve this PR anyway, since none is blocking. I am NOT going to merge this PR yet to give you some time to apply any of my suggestions. However, feel free to ignore them and merge the PR yourself.
...entation/src/main/java/com/automattic/android/experimentation/remote/AssignmentsDtoMapper.kt
Show resolved
Hide resolved
...entation/src/main/java/com/automattic/android/experimentation/remote/AssignmentsDtoMapper.kt
Outdated
Show resolved
Hide resolved
...entation/src/main/java/com/automattic/android/experimentation/remote/ExperimentRestClient.kt
Outdated
Show resolved
Hide resolved
...entation/src/main/java/com/automattic/android/experimentation/remote/ExperimentRestClient.kt
Show resolved
Hide resolved
...tion/src/test/java/com/automattic/android/experimentation/remote/ExperimentRestClientTest.kt
Show resolved
Hide resolved
We were only using this for a few assertions. But we can use assertion method from JUnit, which are just fine for the use cases here.
|
Thank you for the review @ParaskP7 ! I've applied your suggestions, see you in next PRs 🙌 |
2 participants
Description
This PR is a first one of a few that aim to remove
FluxCas a dependency forexperimantationproject.In this PR I introduce
ExperimentRestClientwith logic of parsing fetched JSON and mapping it to domain models.I decided to not introduce a feature branch to keep things simpler: newly introduced classes are not visible to consumers (except domain models, which are useless on its own) so it shouldn't cause confusion.
The logic is based on FluxC implementation, especially
org.wordpress.android.fluxc.network.rest.wpcom.experiments.ExperimentRestClientHow to test?
At this moment, testing is not required or possible due to lack of an entry point. The testing is covered with unit tests, and in next PRs I'll prepare some test scenarios.