search

mattlisiv / newsapi-python

A Python Client for News API
MIT License
376 stars 130 forks source link

add joins for all options that support multiple choices #7

Closed TomFaulkner closed 6 years ago

TomFaulkner commented 6 years ago

Sending an iterable of sources, for example, sends sources=bbc-news,sources=fox-news, this PR does a join on everything that supports multiple entries for a parameter.

Also adds debug level logging for what parameters are being sent to the API.

mattlisiv commented 6 years ago

Thanks for the contribution, but I am not sure this methodology is much more beneficial for the API while making the code more complex.

It seems to me any joining of the sources parameter should happen prior to the function as a seperation of concerns. Are you using it in a particular manner?

Thanks, Matt

TomFaulkner commented 6 years ago

Given Python's lack of static typing the keyword = None tends to imply a list, or other mutable, is intended, and it is accepted and kind of works, but the implementation breaks accepting a list as the API doesn't take the data in the way it is sent.

If a list isn't intended the defaults should probably be empty strings. I didn't document the functions, but doc strings would also be helpful to show the intent.

For how I'm using it, retrieving articles from multiple news sites at a time or using multiple keywords.

TomFaulkner commented 6 years ago

The issue is that you assume it takes an iterable, but instead it takes a string or a list then puts it in, the list isn't handled properly. My PR addresses this. You can use my fork (I changed a few other things, but it should be an easy transition) for this, copy the code from the PR, or format the string yourself.

On Sat, Mar 17, 2018, 2:13 AM Winghin2517 notifications@github.com wrote:

Can I please find out how the existing code actually supports multiple sources? If I type in:

all_articles = newsapi.get_top_headlines(q='bitcoin', sources='bbc-news,the-verge', language='en', page_size=5)

It just returns no results. Whereas if I type into my browser:

https://newsapi.org/v2/top-headlines?sources=bbc-news,the-verge&apiKey=XXX

there are results.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/mattlisiv/newsapi-python/pull/7#issuecomment-373900106, or mute the thread https://github.com/notifications/unsubscribe-auth/AOQ4TKTgfLeFPL0pHFjXMz306czPXBp4ks5tfLeNgaJpZM4Saw2z .

mattlisiv commented 6 years ago

While I respect what you are saying, because the documentation explicitly states "A comma-seperated string of identifiers", I feel that I want to keep the library minimal for now and follow the outlined parameters.

If more people feel strongly about it in the future, we can reopen the issue.

Thanks!