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

Fixed implementation error of emoji.distinct_emoji_lis #156

Merged
merged 3 commits into from Jan 31, 2021

Conversation

daima3629
Copy link
Contributor

This library had a distinct_emoji_lis function to distinct list of emojis from the string, but it didn't work correctly due to an implementation error.
It has been fixed, please review it.

@TahirJalilov
Copy link
Collaborator

@daima3629 Thank's for your PR.
This is really a bug, but your changes do not return the distinct list of emojis from the string. I mean try to check it with the string where the same emoji appears more than once.
I think the best solution to fix this emoji.distinct_emoji_lis() it's just to add a language key.

def distinct_emoji_lis(string, language='en'):
    """Returns distinct list of emojis from the string."""
    distinct_list = list(
        {c for c in string if c in unicode_codes.UNICODE_EMOJI[language]}
    )
    return distinct_list

Could you please, also add to the test_distinct_emoji_lis() assertion to check the distinct of emojis if the same emoji appears more than once? Something like this:
assert emoji.distinct_emoji_lis('Hi, I am fine. 😁😁😁😁') == ['😁']

@daima3629
Copy link
Contributor Author

@TahirJalilov Thank you for your review.
I'll improve on the points you mentioned, please wait a moment.

@daima3629
Copy link
Contributor Author

Fixed problems.
And a test has been changed slightly because the order of the list is not guaranteed due to the use of set.

@TahirJalilov TahirJalilov merged commit 4de8f41 into carpedm20:master Jan 31, 2021
@TahirJalilov
Copy link
Collaborator

Thank you!

@daima3629
Copy link
Contributor Author

You're welcome! Thank you too!

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.

None yet

2 participants