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

Remove Apache dependency to allow for higher Android target SDK. #554

Merged
merged 6 commits into from
Mar 9, 2017
Merged

Remove Apache dependency to allow for higher Android target SDK. #554

merged 6 commits into from
Mar 9, 2017

Conversation

Jawnnypoo
Copy link
Member

This has a chance of resolving #551 though I cannot confirm as the message from Google is vague and does not point out what the issue is specifically. Reguardless, OkHttp 3.X is tried and true, and it seems like it would be a great replacement since the work is already done for it.

This is also a more complete PR of the work done in #418 without depending on beta versions of the SDK tools, plugin, etc.

The change in SDK version required a bump to Robolectric which is why a few of the tests changed as well.

I also added a nice Gradle plugin that allows you to check for the latest dependencies. I attempted to bring all dependencies up to date, but ran into complications with the latest versions of Mockito, so leaving a few of those for another PR.

Since this does drop an HTTP client implementation without deprecation, it may warrant a bump from 1.13.X to 1.14.X, but I leave that up to you. I just think if Google is truly blocking updates and app submissions based on Apache flaws (either coming from this library or otherwise) it is best to just leave it behind since it is deprecated in the SDK anyhow.

@facebook-github-bot
Copy link

By analyzing the blame information on this pull request, we identified @grantland to be a potential reviewer.

@facebook-github-bot
Copy link

@Jawnnypoo updated the pull request - view changes

@facebook-github-bot
Copy link

@Jawnnypoo updated the pull request - view changes

@facebook-github-bot
Copy link

@Jawnnypoo updated the pull request - view changes

@Jawnnypoo
Copy link
Member Author

Sorry for the spammy notifications, should be good to go now. Just had some strange Travis issues.

@Jawnnypoo
Copy link
Member Author

Not really sure what I can do about the code coverage, this PR mostly just removes code, so I don't think it should be a concern

@Jawnnypoo
Copy link
Member Author

Any thoughts on this @grantland ?


testCompile 'org.robolectric:robolectric:3.0'
testCompile 'org.skyscreamer:jsonassert:1.2.3'
testCompile 'org.robolectric:robolectric:3.1.4'
Copy link
Contributor

@rogerhu rogerhu Mar 7, 2017

Choose a reason for hiding this comment

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

bump to 3.3?

@rogerhu
Copy link
Contributor

rogerhu commented Mar 7, 2017

+1 okhttp is now the underlying interface of HttpUrlConnection. Can we get this merged in?

@rogerhu
Copy link
Contributor

rogerhu commented Mar 7, 2017

Issue is related to: #231

@Jawnnypoo
Copy link
Member Author

Yeah I would love to see this moved forward. Supporting only one HttpClient should reduce the headache of supporting this SDK massively.

@Jawnnypoo
Copy link
Member Author

I think @grantland is one of the only devs on this project that has the authority to merge PRs on this repo, but I don't think he is too active on doing so.

@facebook-github-bot
Copy link

@Jawnnypoo updated the pull request - view changes

NotificationCompat.Builder builder = new NotificationCompat.Builder(b.mContext);
builder.setContentTitle(b.mContentTitle);
builder.setContentText(b.mContentText);
builder.setContentIntent(b.mContentIntent);
Copy link
Contributor

Choose a reason for hiding this comment

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

is this going to work without running builder.build() and returning the result?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, updated

@rogerhu
Copy link
Contributor

rogerhu commented Mar 8, 2017

The rest of the PR (minus the notification stuff) looks great, thanks for doing this.

Does anyone else have strong reservations/opinions about dropping the Apache client? Given Android 2.x is 1.0% of the market share (https://developer.android.com/about/dashboards/index.html), it seems like we should do this.

We will bump to v1.14.XX once this happens.

@facebook-github-bot
Copy link

@Jawnnypoo updated the pull request - view changes

@Jawnnypoo
Copy link
Member Author

@rogerhu This doesn't require dropping any support, we can continue to target 9 as the min version, using Okhttp exclusively does not change that number. 👍

@Jawnnypoo
Copy link
Member Author

Also, I fixed the Notification issue, so this should be good to go

@rogerhu
Copy link
Contributor

rogerhu commented Mar 8, 2017

Thanks. I'm not seeing the latest build pushed up to Maven so going to check with Parse guys to figure out how to handle... :)

@flovilmart
Copy link
Contributor

@grantland on that point, could you let us on Sonatype or open a Pr for distribute on tags?

@rogerhu
Copy link
Contributor

rogerhu commented Mar 9, 2017

I suspect that the builds are going up on Sonatype fine (https://books.sonatype.com/nexus-book/reference/staging-repositories.html) but need to be released either manually or via API

@rogerhu rogerhu merged commit aed49e2 into parse-community:master Mar 9, 2017
@Jawnnypoo Jawnnypoo deleted the remove-apache branch March 10, 2017 02:19
natario1 added a commit to natario1/Parse-SDK-Android that referenced this pull request Mar 11, 2017
commit aed49e2
Author: John <jawnnypoo@gmail.com>
Date:   Thu Mar 9 01:43:57 2017 -0600

    Remove Apache dependency to allow for higher Android target SDK. (parse-community#554)

    * Remove Apache dependency to allow for higher Android target SDK.

    * Remove ParseURLConnectionHttpClient in favor of ParseOkHttpClient

    * Add JDK specification to Travis

    * Assure the latest tools are installed

    * Update to the latest dependencies

    * Update so that the notification is built properly

commit 93dbb83
Author: Roger Hu <roger.hu@gmail.com>
Date:   Wed Mar 8 23:26:11 2017 -0800

    Bump to 1.14.1 snapshot

commit 5bacd08
Author: Roger Hu <roger.hu@gmail.com>
Date:   Wed Mar 8 17:36:50 2017 -0800

    Bump to v1.13.3 release

commit 735be49
Author: Mattia <mat.iavarone@gmail.com>
Date:   Thu Mar 9 02:34:25 2017 +0100

    Closing streams in ParseAWSRequest (parse-community#590)

    * Update ParseAWSRequest.java

commit 3fced98
Author: Florent Vilmart <flovilmart@users.noreply.github.com>
Date:   Wed Mar 8 20:33:32 2017 -0500

    Create .codecov.yml (parse-community#594)

commit ea489eb
Merge: 5434260 b6c98bd
Author: Roger Hu <roger.hu@gmail.com>
Date:   Wed Mar 8 12:15:29 2017 -0800

    Merge pull request parse-community#592 from rogerhu/1.13.3

    Bump to v1.13.3-SNAPSHOT

commit b6c98bd
Author: Roger Hu <roger.hu@gmail.com>
Date:   Wed Mar 8 12:14:17 2017 -0800

    Bump to v1.13.3-SNAPSHOT

commit 5434260
Merge: 64e74dc 2f2d856
Author: Roger Hu <roger.hu@gmail.com>
Date:   Wed Mar 8 12:10:59 2017 -0800

    Merge pull request parse-community#591 from rogerhu/1.13.2

    Bump to v1.13.2 release

commit 2f2d856
Author: Roger Hu <roger.hu@gmail.com>
Date:   Wed Mar 8 12:08:55 2017 -0800

    Bump to v1.13.2 release

commit 64e74dc
Merge: fe08345 f78a722
Author: Roger Hu <roger.hu@gmail.com>
Date:   Wed Mar 8 12:06:09 2017 -0800

    Merge pull request parse-community#506 from posativ/patch-1

    only add client key if non-null

commit f78a722
Author: Martin Zimmermann <zimmermann@weloveapps.de>
Date:   Wed Jul 27 14:29:25 2016 +0200

    only add client key if non-null
@rogerhu
Copy link
Contributor

rogerhu commented Mar 11, 2017

v13.3.3 is released thanks to @flovilmart. Apparently the release process is not quite automated on Travis yet but is being worked on.

This PR has been merged into v1.14.1-snapshot. Would appreciate people check and see if there are any lingering issues.

@rogerhu
Copy link
Contributor

rogerhu commented Mar 17, 2017

v1.14.0 is now out which should include these changes.

@Jawnnypoo
Copy link
Member Author

Awesome! Thanks!

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.

4 participants