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.

Sign up for GitHub

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

Jump to bottom

Added EmojiUtils #145

Merged
merged 10 commits into from
Jun 26, 2017
Merged

Added EmojiUtils #145

merged 10 commits into from
Jun 26, 2017

Conversation

aballano
Copy link
Contributor

This class contains 2 methods for now:

  • isOnlyEmojis: Returns true wether the given text only contains emojis (whitespaces are ignored)
  • emojisCount: Returns the number of emojis for the given text.

Alberto Ballano added 2 commits June 23, 2017 16:47
vanniktech
vanniktech requested changes Jun 23, 2017
View reviewed changes
Copy link
Owner

@vanniktech vanniktech left a comment

Choose a reason for hiding this comment

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

Few things otherwise this looks good

emoji/src/main/java/com/vanniktech/emoji/EmojiHandler.java Outdated
@@ -2,7 +2,9 @@

import android.content.Context;
import android.text.Spannable;

Copy link
Owner

Choose a reason for hiding this comment

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

nit: remove these

emoji/src/main/java/com/vanniktech/emoji/EmojiHandler.java Outdated
import com.vanniktech.emoji.EmojiManager.EmojiRange;

Copy link
Owner

Choose a reason for hiding this comment

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

nit: remove these

emoji/src/main/java/com/vanniktech/emoji/EmojiManager.java Outdated
import com.vanniktech.emoji.emoji.Emoji;
import com.vanniktech.emoji.emoji.EmojiCategory;

Copy link
Owner

Choose a reason for hiding this comment

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

nit: remove these

emoji/src/main/java/com/vanniktech/emoji/EmojiManager.java Outdated
@@ -2,8 +2,10 @@

import android.support.annotation.NonNull;
import android.support.annotation.Nullable;

Copy link
Owner

Choose a reason for hiding this comment

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

nit: remove these

emoji/src/main/java/com/vanniktech/emoji/EmojiUtils.java Outdated

public final class EmojiUtils {

private static final Pattern SPACE_REMOVAL = Pattern.compile(" ");
Copy link
Owner

Choose a reason for hiding this comment

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

what about tabs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also added line terminations to get rid of the trim()

emoji/src/test/java/com/vanniktech/emoji/EmojiUtilsTest.java Outdated
import com.pushtorefresh.private_constructor_checker.PrivateConstructorChecker;
import com.vanniktech.emoji.emoji.Emoji;
import com.vanniktech.emoji.emoji.EmojiCategory;

Copy link
Owner

Choose a reason for hiding this comment

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

nit: remove

emoji/src/test/java/com/vanniktech/emoji/EmojiUtilsTest.java Outdated
package com.vanniktech.emoji;

import android.support.annotation.NonNull;

Copy link
Owner

Choose a reason for hiding this comment

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

nit: remove

emoji/src/test/java/com/vanniktech/emoji/EmojiUtilsTest.java Outdated

@Before public void setUp() {
EmojiManager.install(new EmojiProvider() {
@NonNull
Copy link
Owner

Choose a reason for hiding this comment

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

nit: same line

emoji/src/test/java/com/vanniktech/emoji/EmojiUtilsTest.java Outdated
}

@Test public void isOnlyEmojis_empty() {
boolean onlyEmojis = EmojiUtils.isOnlyEmojis("");
Copy link
Owner

Choose a reason for hiding this comment

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

just inline all of these variables please

emoji/src/test/java/com/vanniktech/emoji/EmojiUtilsTest.java Outdated
.check();
}

@Test public void isOnlyEmojis_empty() {
Copy link
Owner

Choose a reason for hiding this comment

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

also remove underscores from method names

rubengees
rubengees reviewed Jun 23, 2017
View reviewed changes
emoji/src/main/java/com/vanniktech/emoji/EmojiUtils.java
import java.util.regex.Matcher;
import java.util.regex.Pattern;

public final class EmojiUtils {
Copy link
Collaborator

@rubengees rubengees Jun 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think the EmojiManager should provide this functionality and not another class.

@vanniktech Maybe we should make all of the EmojiManager public now as with this included changes it is quite usefull for other things apart from only managing TextViews and EditTexts (#128).

Copy link
Owner

Choose a reason for hiding this comment

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

Let's leave it here for now (in EmojiUtils) I anyways thought of restructuring it a bit before the 0.5.0 release.

vanniktech
vanniktech approved these changes Jun 24, 2017
View reviewed changes
Copy link
Owner

@vanniktech vanniktech left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good now. Just the build would need to get green

@aballano
Copy link
Contributor Author

@vanniktech

[ant:checkstyle] [ERROR] /home/travis/build/vanniktech/Emoji/emoji/src/test/java/com/vanniktech/emoji/EmojiUtilsTest.java:70:50: The String "hello" appears 8 times in the file. [MultipleStringLiterals]

Really? :D

@aballano
Copy link
Contributor Author

@vanniktech I added a suppression for that MultipleStringLiterals in tests, let me know if that's not ok.

@codecov
Copy link

codecov bot commented Jun 26, 2017

Codecov Report

Merging #145 into master will increase coverage by 2%.
The diff coverage is 86.66%.

@@           Coverage Diff           @@
##           master     #145   +/-   ##
=======================================
+ Coverage    24.2%   26.21%   +2%     
=======================================
  Files          19       20    +1     
  Lines         661      679   +18     
  Branches       72       72           
=======================================
+ Hits          160      178   +18     
  Misses        490      490           
  Partials       11       11

vanniktech
vanniktech approved these changes Jun 26, 2017
View reviewed changes
Copy link
Owner

@vanniktech vanniktech left a comment

Choose a reason for hiding this comment

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

All good, thank you very much !

@vanniktech vanniktech merged commit ac35d0b into vanniktech:master Jun 26, 2017
@vanniktech vanniktech mentioned this pull request Jun 26, 2017
Closed
@aballano aballano deleted the ab/emojiutils branch June 26, 2017 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rubengees rubengees rubengees left review comments

@vanniktech vanniktech vanniktech approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aballano @vanniktech @rubengees