Migration to GitHubSdk 0.5 #1158
Conversation
There was a problem hiding this comment.
Have a look at the comments I've added.
Also every file were you've only change imports or spaces should be it's own PR. It's really hard seeing what actually changed when there is 10 files of only changing import order before actual changes.
Another point is that the Kotlin change should have been it's own PR since I'm guessing that the only thing that changed was some method calls. It's fine to leave it in but try to not cram to many things in to one PR.
| @@ -400,8 +400,8 @@ public void onDialogResult(int requestCode, int resultCode, Bundle arguments) { | |||
| // Update comment list | |||
| if (comments != null) { | |||
| int position = Collections.binarySearch(comments, | |||
| comment, (lhs, rhs) -> Integer.valueOf(lhs.id()) | |||
| .compareTo(rhs.id())); | |||
| comment, (lhs, rhs) -> Integer.valueOf(lhs.id().intValue()) | |||
There was a problem hiding this comment.
Why not compare the Long instead?
| @@ -26,7 +26,15 @@ | |||
| import java.util.HashMap; | |||
| import java.util.Map; | |||
|
|
|||
| import static com.github.pockethub.android.core.issue.IssueFilter.*; | |||
There was a problem hiding this comment.
This does not look good at all. I will declutter this and any other static imports
| .build(); | ||
|
|
||
| event = GitHubEvent.builder() | ||
| .repo(new GitHubEvent.RepoIdentifier() { |
There was a problem hiding this comment.
This need to be fixed, I'll get to today. GitHubEvent.RepoIdentifier should really have a builder.
There will be a new release today.
There was a problem hiding this comment.
Version 0.5.1 has a builder for this and is released now.
There was a problem hiding this comment.
Ok great, a builder will help a lot.
|
Thanks for the review. I will fix the tests for 0.5.1 and optimize some of the other things you mentioned as soon as I can. Will also keep PRs simple in the future. |
|
I made the changes for the minor comments. I have the migration to 0.5.1 with all the tests passing on a feature branch. I'm assuming you'd prefer that as a separate PR, otherwise I can push the changes here. Let me know. |
|
Well the changes to 0.5.1 would probably be nice to have in this since it's just for the |
This reverts commit 8c448e2.
|
Ok, this should do it then. |
|
Looks good. |
I thought it might be a good idea to connect the app to the new version of the SDK, which is much more flexible and will allow for more features to be added later, following up from Meisolsson/GitHubSdk#13. There are a couple of breaking changes and I have rewritten a number of classes and tests to take that into account. Because classes in
com.github.pockethub.android.ui.item.newshave been the most heavily affected, I also decided to migrate that package to Kotlin.