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

Google Guava over Apache Commons Lang3 #3016

Merged
merged 5 commits into from
Jul 18, 2017
Merged

Google Guava over Apache Commons Lang3 #3016

merged 5 commits into from
Jul 18, 2017

Conversation

koppor
Copy link
Member

@koppor koppor commented Jul 15, 2017

We don't have an Architectural Decision Record why we included Apache Commons Lang. We introduced the dependency within #1572.

I don't know, why Apache Commons Lang3 was chosen. The current opinion is that Guava is more modern and that new projects should use Google Guava instead of Apache Commons Lang3.

Should we replace Apache Commons Lang3 or should we just keep it, because it is not that huge and we have other class issues (see #3014).

Refs #2966 (comment)

Update

  • I replaced most places where we use Apache Commons Lang3 and there is a Google Guava equivalent
  • We have some places, where there is no Google Guava equivalent, so I kept that
  • Moved all architecture tests to org.jabref.architecture.
  • I began to use ArchUnit for checking that. However, I have issues in using the notAnnotatedWithRule (see NPE demonstration TNG/ArchUnit#19).

@koppor koppor added type: code-quality Issues related to code or architecture decisions type: question status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Jul 15, 2017
@Siedlerchr
Copy link
Member

I like the guava stuff. I don't see any big advantage in the apache commons stuff which the Guava has not.
And it seems like the Guava has also an Escape method:
https://github.com/google/guava/blob/9b94fb3965c6869b0ac47420958a4bbae0b2d54c/android/guava-tests/test/com/google/common/html/HtmlEscapersTest.java

@lenhard
Copy link
Member

lenhard commented Jul 15, 2017

I am also in favor of replacing commons lang with guava. So, if anybody wants to go ahead with this, be my guest.

@koppor
Copy link
Member Author

koppor commented Jul 16, 2017

Fun fact:

+--- com.microsoft.azure:applicationinsights-core:1.0.+ -> 1.0.8
|    +--- eu.infomas:annotation-detector:3.0.4
|    +--- commons-io:commons-io:2.4
|    +--- org.apache.commons:commons-lang3:3.1 -> 3.4
|    +--- org.apache.httpcomponents:httpclient:4.3.5 -> 4.5.2 (*)
|    \--- com.google.guava:guava:12.0.1 -> 22.0 (*)

@koppor
Copy link
Member Author

koppor commented Jul 16, 2017

HtmlToUnicode is unfortunately not the same as Escaping HTML:

Ε gets Ε instead of E 😄

@koppor
Copy link
Member Author

koppor commented Jul 16, 2017

What I stumbled upon while searching for various commons lang replacements: Eclipse Collections.

grafik

Maybe, we can reduce our memory by using Eclipse Collections?

I continue that discussion at #3023

@koppor koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jul 16, 2017
@koppor koppor changed the title Google Guava over Apache Commons Lang3 [WIP] Google Guava over Apache Commons Lang3 Jul 16, 2017
@koppor
Copy link
Member Author

koppor commented Jul 16, 2017

  • I replaced most places where we use Apache Commons Lang3 and there is a Google Guava equivalent
  • We have some places, where there is no Google Guava equivalent, so I kept that
  • I began to use ArchUnit for checking that. However, I have I had issues in using the notAnnotatedWithRule (see NPE demonstration TNG/ArchUnit#19), but these are resolved now.

The ArchRule doNotUseApacheCommonsLang3 reads easy:

public static final ArchRule doNotUseApacheCommonsLang3 =
        noClasses().that().areNotAnnotatedWith(ApacheCommonsLang3Allowed.class)
        .should().accessClassesThat().resideInAPackage("org.apache.commons.lang3");

However, this rule completely ignores the annotation ApacheCommonsLang3Allowed and I have no clue how to solve that.

@koppor koppor added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jul 18, 2017
@koppor koppor changed the title [WIP] Google Guava over Apache Commons Lang3 Google Guava over Apache Commons Lang3 Jul 18, 2017
Copy link
Member

@LinusDietz LinusDietz left a comment

Choose a reason for hiding this comment

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

I really like how ArchUnit works and we should consider using it for more purposes.

I think the PR is good to go for now, although I actually dislike that we don't actually get rid of Commons Lang and even have a annotation for that.
I would suggest to open a issue on that with a beginner label that somebody can have a look how to get rid of the last Commons Lang usages.

@koppor koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jul 18, 2017
@koppor koppor merged commit 1116331 into master Jul 18, 2017
@koppor koppor deleted the guava-first branch July 18, 2017 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: code-quality Issues related to code or architecture decisions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants