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

WIP: remove guava #8528

Closed
wants to merge 1 commit into from
Closed

WIP: remove guava #8528

wants to merge 1 commit into from

Conversation

schmitch
Copy link
Contributor

@schmitch schmitch commented Jul 20, 2018

Warning this PR is not 100% done yet and is just a preview (still needs to remove some occurences) (i just wanted to get it out, since it floated around on my computer for some time)

The hardest part of removing guava will be the following two classes (which is not done yet):

  • FinalizablePhantomReference
  • FinalizableReferenceQueue

and maybe

  • BaseEncoding (which implements our Hex Encoder, shouldn't be too hard...)

Everything else is just replacing the stuff from guava, with stuff that we already have in our framework, like CharStreams or some kind of Primitives checks that we have in some kind of different form.

Purpose

removes guava
currently I think guava is one of the greatest library of all the time. however it's freaking aweful if a framework depend on it, since first of all guava is quite big (2.6mb), second we only use like 2-3 classes from it and the InetAddress stuff, so that we do not need to rely on the Java InetAddress which sometimes might try to resolve a ip/host (unnecessarily), for that I pulled in a new scala library from comcast, called ip4s which makes ip parsing, etc quite easy and comes with a relativ small size <200kb. and last it means that sometimes upgrades to guava are impossible in user code.

we also use a lot of Immutable* however the Maps aren't really needed since they won't leak into user code and the ImmutableList stuff can be replaced by java8 code that makes list immutable

This PR helps that people who need guava can actually use the latest and greatest guava and probably reduce the size of play for all scala users that might never need guava at all.

Copy link
Member

@marcospereira marcospereira left a comment

Choose a reason for hiding this comment

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

Looking good.

The build is failing with a compilation error though.

@@ -52,7 +50,7 @@ public Twitter(WSClient ws) {

public Result auth() {
String verifier = request().getQueryString("oauth_verifier");
if (Strings.isNullOrEmpty(verifier)) {
if (verifier == null || verifier == "") {
Copy link
Member

Choose a reason for hiding this comment

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

verifier.isEmpty.

Choose a reason for hiding this comment

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

verifier.isEmpty will yield NullPointerException if verifier == null is true

Copy link
Contributor

Choose a reason for hiding this comment

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

Apache Commons?

def withTempDir[T](block: File => T) = {
val temp = GFiles.createTempDir()
val temp = Files.createTempDirectory(System.currentTimeMillis() + "-1")
Copy link
Member

Choose a reason for hiding this comment

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

😌

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well GFIles.createTempDir tried to create 5 temp dirs before failing, I guess we can omit that behavior, but maybe I create a small code change for that behavior or I keep guava as Test scoped, but I didn't decided yet which is the best choice.

Copy link
Member

Choose a reason for hiding this comment

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

Since it is test code, I would say to keep your change and if it becomes a problem we can revisit it.

@wsargent
Copy link
Member

For FinalizablePhantomReference and FinalizableReferenceQueue -- those are connected to the temporaryfile reaper. You may be able to export the reaper into a play submodule and that way it's optional.

@@ -99,7 +99,7 @@ public final Self configure(Map<String, Object> conf) {
* @return a copy of this builder configured with the key=value
*/
public final Self configure(String key, Object value) {
return configure(ImmutableMap.of(key, value));
return configure(Collections.singletonMap(key, value));

Choose a reason for hiding this comment

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

Nice

@@ -4,7 +4,6 @@

package play.inject.guice;

import com.google.common.collect.ImmutableMap;
import com.google.inject.Module;
Copy link

@rakeshhny rakeshhny Aug 7, 2018

Choose a reason for hiding this comment

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

Should this com.google.inject.Module also be removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

@rakeshhny Module is the part of Guice, not Guava

@marcospereira
Copy link
Member

@schmitch do you still want this to land in Play 2.7?

@schmitch
Copy link
Contributor Author

sadly I do not have time to complete that for 2.7 :(

@ignasi35 ignasi35 added this to the Play 2.7.0 milestone Oct 12, 2018
@ignasi35
Copy link
Member

Tentatively adding the milestone 2.7.0.

@marcospereira marcospereira removed this from the Play 2.7.0 milestone Nov 1, 2018
@marcospereira
Copy link
Member

Removing from 2.7.0 milestone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants