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

Add NumberOfHashtags and NumberOfMentions primitives #180

Merged
merged 35 commits into from
Aug 2, 2022
Merged

Conversation

sbadithe
Copy link
Contributor

@sbadithe sbadithe commented Jul 22, 2022

@sbadithe sbadithe changed the title Add Number Of Hashtags primitive Add NumberOfHashtags and NumberOfMentions primitives Jul 22, 2022
@gsheni gsheni requested review from rwedge and a team and removed request for rwedge July 22, 2022 22:29
Copy link
Contributor

@thehomebrewnerd thehomebrewnerd left a comment

Choose a reason for hiding this comment

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

I have a few comments about what we consider valid hashtags and mentions. If we make changes to the regex, we'll also need to update the associated tests accordingly.

Aside from some questions about what is a valid hashtag or mention, this PR looks pretty good to me.

nlp_primitives/number_of_hashtags.py Outdated Show resolved Hide resolved
nlp_primitives/number_of_hashtags.py Outdated Show resolved Hide resolved
nlp_primitives/number_of_mentions.py Outdated Show resolved Hide resolved
nlp_primitives/number_of_mentions.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 28, 2022

Codecov Report

Merging #180 (e83a14f) into main (11837a5) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #180      +/-   ##
==========================================
+ Coverage   99.08%   99.16%   +0.08%     
==========================================
  Files          43       47       +4     
  Lines        1089     1199     +110     
==========================================
+ Hits         1079     1189     +110     
  Misses         10       10              
Impacted Files Coverage Δ
nlp_primitives/__init__.py 100.00% <100.00%> (ø)
nlp_primitives/number_of_hashtags.py 100.00% <100.00%> (ø)
nlp_primitives/number_of_mentions.py 100.00% <100.00%> (ø)
nlp_primitives/tests/test_number_of_hashtags.py 100.00% <100.00%> (ø)
nlp_primitives/tests/test_number_of_mentions.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us.

@gsheni
Copy link
Contributor

gsheni commented Aug 1, 2022

@sbadithe What is the status of this? Ready for another review?

@sbadithe
Copy link
Contributor Author

sbadithe commented Aug 1, 2022

@sbadithe What is the status of this? Ready for another review?

Still working on this. Trying to get all the edge cases to work (as well as Unicode) by referencing Twitter's open source implementation.

Copy link
Contributor

@thehomebrewnerd thehomebrewnerd left a comment

Choose a reason for hiding this comment

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

Looks pretty good, just a few wording suggestions. Also, the comments that I left on the docstrings apply to both primitives, although I left them in only one place.

nlp_primitives/number_of_hashtags.py Outdated Show resolved Hide resolved
nlp_primitives/number_of_mentions.py Outdated Show resolved Hide resolved
nlp_primitives/number_of_hashtags.py Outdated Show resolved Hide resolved
nlp_primitives/number_of_hashtags.py Outdated Show resolved Hide resolved
nlp_primitives/number_of_hashtags.py Outdated Show resolved Hide resolved
nlp_primitives/number_of_hashtags.py Outdated Show resolved Hide resolved
@sbadithe sbadithe merged commit 399cfc2 into main Aug 2, 2022
@sbadithe sbadithe deleted the NumberOfHashtags branch August 2, 2022 18:33
@thehomebrewnerd thehomebrewnerd mentioned this pull request Sep 14, 2022
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.

Add NumberOfMentions primitive Add NumberOfHashtags primitive
3 participants