More injection for WeatherApi#8
Conversation
| } | ||
| buildTypes.each { | ||
| it.buildConfigField 'String', 'WEATHER_API_KEY', "\"<YOUR WEATHER_API_KEY>\"" | ||
| it.buildConfigField 'String', 'WEATHER_API_KEY', "\"c1302d276960198b7803920222ab1dfb\"" |
There was a problem hiding this comment.
Lets make sure we dont expose your api key here ;)
| class HttpClientProvider @Inject constructor(@Named("application") var application: Application) : Provider<OkHttpClient> { | ||
|
|
||
| companion object { | ||
| private val sizeOfHttpCache: Long = 10 * 1024 * 1024 // 10 MiB |
There was a problem hiding this comment.
Fix up the naming here so we have uppercase underscore naming instead.
SIZE_OF_CACHE
| clientBuilder.addNetworkInterceptor(CacheControlInterceptor(application)) | ||
| return clientBuilder.build() | ||
| } | ||
| } No newline at end of file |
| .addConverterFactory(retrofit2.converter.moshi.MoshiConverterFactory.create()) | ||
| .build() | ||
| } | ||
| } No newline at end of file |
| bind(CompositeDisposable::class.java).toProviderInstance(CompositeDisposableProvider()) | ||
|
|
||
| bind(String::class.java).withName("url").toInstance(WeatherService.BASE_URL) | ||
| bind(OkHttpClient::class.java).withName("okHttpClient").toProviderInstance(HttpClientProvider(application)) |
There was a problem hiding this comment.
Not so sure we need the @nAmed injection here as we are utilizing only one provider / instance in this app of the OkHttpClient, Retrofit instance and WeatherAPI. Is there a benefit here for named injection that Im missing ?
| } | ||
| dependencies { | ||
| classpath 'com.android.tools.build:gradle:2.2.3' | ||
| classpath 'com.android.tools.build:gradle:3.1.0-alpha01' |
There was a problem hiding this comment.
Lets just bump this to 3.0.0 to avoid having to use an alpha version
|
Thanks for this PR @afaucogney ! A few questions regarding the named injections, as I feel just normal providers without named instances in this case should do the trick. Besides that we should just upgrade to GAP 3.0.0 to avoid using an alpha version. Again thanks a lot this looks great! Let me know what you think, Ill make sure to get back to you a lot faster this time, havent have too much time to check this repo. |
3 participants
As discussed in this issue stephanenicolas/toothpick#244, here is a version with more injections for the WeartherApi.
The WeatherApi just inject the RetrofitClient and its recursive dependencies. I'm not sure this is the best way of doing with TP and Retrofit, but I think this is a good example to discuss and then push the best solution.
This PR also update the dep and sdk versions