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

Added universal search function which passes testing #148

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

Conversation

sean-bailey
Copy link

This was a really simple addition! By taking the existing hashtag and user query search functions, I copied the guts from those and now added the ability for users to search for any string they want on Twitter using the same API.

Added proper documentation, unit testing, and output.

@bisguzar
Copy link
Owner

Awesome! Thanks for your support, I was thinking about it. But we should use get_tweets function instead creating new one , in my opinion. We can get search filters as args, what do you think?

@sean-bailey
Copy link
Author

I considered that! I was concerned about the limit of arguments in the get_tweets as it was either hashtag or user. How about this: Have two different arguments: "searchterm" and "username", that way we keep a single "get_tweets()" but get the functionality of focusing on either user profiles or "everything else", which naturally would encapsulate the hashtag? That's a pretty quick and easy modification. What do you think @bisguzar ?

@bisguzar
Copy link
Owner

bisguzar commented Jun 21, 2020

Sounds good, we just can not break current API, for backward compatibility. İts easy to handle hashtags and usernames, because of first char (#) of hashtags. The other args (like contains, until, since, etc...) will be optional. If they are defined, we will progress it on search URL, otherwise current function handle it.

I'm looking for your commits exitedly!

@sean-bailey
Copy link
Author

Perfect! I removed the separate search function as we discussed and integrated it in to the existing get_tweets functionality, adding a bit of error handling while preserving the previous API backwards compatibility. This way existing software using the function still works normally, and the new search capabilities can be leveraged in future programs. How does it look, @bisguzar ?

@bisguzar
Copy link
Owner

Hi again Sean, and thanks again ofc... It is looking great! I'm sorry for latency. Just having busy days... I saw two things, I want to tell these things. First, the tests still exists from previous function. It doesn't matter anything. I can delete it before merging, nvm. The second one is, name of parameters. In python, we are don't like camelCase in variable names as you know.

I will test your changes ASAP

@sean-bailey
Copy link
Author

No camelCase? No problem! Switched it to underscore_case for extended variables (and to prevent a conflict with existing variable username)

@ZebTheWizard
Copy link

I am also excited for this feature!
Can this be pulled yet?

@bisguzar
Copy link
Owner

bisguzar commented Jul 16, 2020

Ah, I know guys. I'm pretty fast...
I reviewed and tested all of your feature @sean-bailey, finally, thanks!!!
I just refactored a bit and pushed the changes to your fork. Can you review them?

Copy link
Author

@sean-bailey sean-bailey left a comment

Choose a reason for hiding this comment

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

I like the raise RuntimeError responses to the query and search! Excellent. This is approved on my end.

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

3 participants