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 Search feature to Youtube Scraper #194

Merged
merged 2 commits into from
Aug 25, 2021

Conversation

OmarELRayes
Copy link
Contributor

@OmarELRayes OmarELRayes commented Aug 22, 2021

Added Search feature with keyword with a corresponding test using assertPosts.
Also fixed an issue where video's views would be in this format 150,100,100 format (with commas).
One problem we need to address that the views property in PostStatistics is Int? where there are videos that exceeds that limit, didn't want to change it as it's tightly coupled with the rest of the scrapers.

Copy link
Owner

@sokomishalov sokomishalov left a comment

Choose a reason for hiding this comment

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

Thank you very much for your contribution! Really looking forward to this feature.

One little thing - current structure prefers common fetchPosts method with bunch of extensions for provider-specific scrapers, which incapsulates path and query params building. From my perspective it is quite relative case.
Regarding issue with Int restrictions - I haven't met yet cases when size exceeds integer boundaries, but if it is - it'a not a big deal to increase it up to long boundaries, sure. It possibly would break compatibility, but anyway this tool definitely will never have major first version due to specifics of scraping and changing document structure of data providers without any notice.

@OmarELRayes
Copy link
Contributor Author

Love to contribute in such a good library and clean code!
I understand your point, I made it as a separate function to emphasis "separation of concerns" ( pardon if I'm mistaken, still a junior dev here :D ).
Can you elaborate more on what I need to change ?
the case is I need to the user to pass me a "keyword" value instead of a path because the path is pretty much stable "/results" then I add the keyword to the queryParams map.

@sokomishalov
Copy link
Owner

Yes, I got your point.
You should just convert this public function to the extension function in YoutubeSkraperExtensions.kt.
Signature would smth like this (yes, I also should rename path param to urn):

fun YoutubeSkraper.getSearchPosts(keyword: String): Flow<Post> {
    return getPosts(path = "/results?search_query=${keyword.replace(" ","+")}")
}

And your function logic should be inside getPosts() to be capable of scraping both user videos and search results videos.

@OmarELRayes
Copy link
Contributor Author

Thank you for elaboration, It's much cleaner now.
Hope you find it better.

@sokomishalov sokomishalov merged commit acfb307 into sokomishalov:master Aug 25, 2021
@sokomishalov
Copy link
Owner

Thank you, that's exactly what I meant!
It's merged.

sokomishalov added a commit that referenced this pull request Aug 25, 2021
sokomishalov added a commit that referenced this pull request Aug 25, 2021
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