Skip to content
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

Dagger android #5

Merged
merged 21 commits into from
Dec 22, 2017
Merged

Dagger android #5

merged 21 commits into from
Dec 22, 2017

Conversation

alvarez-fabian
Copy link
Contributor

@alvarez-fabian alvarez-fabian commented Jul 24, 2017

Summary

Update wolmo-networking to make it compatible with dagger.

  • Add dagger dependencies as provided.
  • Create new NetworkComponent to build and configure RetrofitServices.
  • Add GsonModule to provide a Gson instance and dependencies to build it.
  • Add NetworkingModule to provide a Retrofit instance.
  • Add OkHttpClientModule to provide OkHttpClient, and add interceptors to it.
  • Remove NetworkingApplication that's not needed anymore.
  • Update RetrofitServices to move all the creation and configuration to the specific modules.

Description

Applications that need to use wolmo-network doesn't need anymore to extend NetworkingApplication.
This library provides the modules to build and configure the dependencies of retrofit, Gson and OkHttp, it also provides a default NetworkingComponent to simplify the creation and reduce configuration on common scenarios. For it to work you only need to provide it the baseUrl, an optional list of interceptors to add to okHttp and an optional list of gson type adapters.

We can find the following modules:

  • GsonModule:
  • OkHttpClientModule
  • NetworkingModule

NetworkingComponent is a template component for simple uses, if the clients want to further customize it they can make a new Component and include the modules provided with this library or make their own.
If you think we can separate this modules more please let me know so we can make this more modular.

All the modules and the component in this library uses @NetworkingScope to define a common scope.

TODO

  • We should start adding unit tests.
  • In the near future we should add some integration testing too. I don't think we need specific android instrumented tests here, since this library does not have any specific dependency on android life cycle.

Copy link
Contributor

@SpicyCactuar SpicyCactuar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of comments, but it looks cleeeeeeeean!

dependencies {
compile fileTree(dir: 'libs', include: ['*.jar'])

// Wolmo
compile 'com.github.Wolox:wolmo-core-android:v1.0.0'
compile 'com.github.Wolox:wolmo-core-android:dagger-android-SNAPSHOT'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should change the dagger-android-SNAPSHOT to the corresponding version when merging the PR. Maybe put a TODO 🤔?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch, I'll add it 😄

*/
public abstract class NetworkingApplication extends WolmoApplication {
public class LoggingUtils {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we build a class just to return an HttpLoggingInterceptor?

If it provided several different Interceptors or something like that, i would be more in favor. But i believe this code can be inlined.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'll remove this class and let the client to create and configure a logger interceptor if they want. Do you think we should keep the library com.squareup.okhttp3:logging-interceptor?

@alvarez-fabian
Copy link
Contributor Author

@emalamela Done 😄

Copy link
Contributor

@SpicyCactuar SpicyCactuar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job!

@SpicyCactuar SpicyCactuar removed their assignment Sep 1, 2017
@alvarez-fabian
Copy link
Contributor Author

@alvarez-fabian alvarez-fabian merged commit a73ca53 into development Dec 22, 2017
@alvarez-fabian alvarez-fabian deleted the dagger-android branch December 22, 2017 19:33
alvarez-fabian pushed a commit that referenced this pull request Dec 22, 2017
alvarez-fabian pushed a commit that referenced this pull request Dec 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants