-
Notifications
You must be signed in to change notification settings - Fork 156
Conversation
@flipper83 use this PR to publish your progress on the sample project please |
Conflicts: rosie/src/main/java/com/karumi/rosie/repository/PaginatedRosieRepository.java rosie/src/main/java/com/karumi/rosie/repository/datasource/paginated/EmptyPaginatedCacheDataSource.java rosie/src/main/java/com/karumi/rosie/repository/datasource/paginated/PaginatedReadableDataSource.java rosie/src/main/java/com/karumi/rosie/repository/datasource/paginated/PaginatedWriteableDataSource.java rosie/src/test/java/com/karumi/rosie/repository/PaginatedRosieRepositoryTest.java sample/src/main/java/com/karumi/rosie/sample/characters/repository/datasource/CharactersApiDataSource.java sample/src/main/java/com/karumi/rosie/sample/comics/domain/usecase/GetComicSeriesPage.java sample/src/main/java/com/karumi/rosie/sample/comics/repository/datasource/ComicSeriesApiDataSource.java sample/src/main/java/com/karumi/rosie/sample/comics/view/presenter/ComicsPresenter.java
Update pull request with marvel characters api |
import com.karumi.rosie.sample.main.ApplicationModule; | ||
import javax.inject.Inject; | ||
import javax.inject.Named; | ||
|
||
public class CharactersRepository extends PaginatedRosieRepository<String, Character> { | ||
|
||
@Inject public CharactersRepository(CharactersApiDataSource apiDataSource, | ||
@Inject public CharactersRepository(CharacterDataSourceFactory characterDataSourceFactory, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for choose fake data or valid values data I created this factory that return one or other. do you like it guys?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of do this why don't we move the factory to the dependency injector? The code will be clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some similar to this one?
@Provide
public CharacterDataSource provideCharacterDataSource() {
if(hasKeys) {
ApiClient apiclient = new ApiClient(...)
return new CharacterApiDataSource(apiClient)
} else {
return new CharacterFakeDataSource()
}
}
??? I did it, but I believed that hide the implementation for the people that read this part. If both of you think that is better I changed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer this option. If you've configured the marvel api credentials or not is a config issue and the dependency injector is used to provide different implementations based on the configuration. IHMO we can move it there and remove the factory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, @Serchinastico what do you prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After told @Serchinastico @pedrovgs and I we decided use the factory class.
Good job, are we planning to use the Marvel Api Client for comics too? (:+1:) |
compile 'com.karumi:dividers:1.0.3' | ||
compile 'com.victor:lib:1.0.1' | ||
compile 'com.victor:lib:1.0.1' // spinner |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this comment? If we are going to add a comment we should add the repository url https://github.com/yankai-victor/Loading or remove it at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I vote for remove it. @Serchinastico this code come from you, any reason for not remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd put the url, the library name is everything but descriptive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok I changed for the url
PR updated please review the unified pagedatasource. |
PaginatedReadableDataSource<String, Character> characterDataSource = | ||
characterDataSourceFactory.createDataSource(); | ||
addReadableDataSources(characterDataSource); | ||
addPaginatedReadableDataSources(characterDataSource); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please review this issue I've created, we should fix it in the next PR #34
public abstract class MarvelActivity extends RosieActivity { | ||
@Override protected abstract int getLayoutId(); | ||
|
||
@Nullable @OnClick(R.id.iv_toolbar_back) public void onBackButtonClicked() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nullable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, in butterkinfe 2, Optional is now Nullable, and Inject is Bind. Jake style!
👍 |
This branch includes some minor changes anticipating what we talked with Luis for the new designs