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

modified get_emoticons util functions #39

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

modified get_emoticons util functions #39

wants to merge 3 commits into from

Conversation

ChillarAnand
Copy link
Contributor

used split() instead of regex to extract emoticons.

@Ffisegydd
Copy link
Member

Thanks for this PR Chillar. I hope you've had a good Christmas break :)

I'd say that rather than returning a set of the emoticons, we should return a dictionary with the count of how many times the emoticon is used.

So it may return, for example:

get_emoticons(" This is my text :D :D :D but it has no code :(")
# {':D': 3, ':(':1}

This would allow us to take into account over-use of smileys etc, but also mean we could get the same set of emoticons using dict.keys() if needs be.

I'm planning on doing some more Nidaba over the next few days.

@Ffisegydd
Copy link
Member

We also need much more thorough unittests. Add more if you can, but if you can't think of any more thorough ones then just PR it and I'll add a few more before merging.

@ChillarAnand
Copy link
Contributor Author

get_emoticons accepts string or list of words. modified unit tests.

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.

2 participants