search

MycroftAI / skill-npr-news

Mycroft AI official News Skill, providing the latest news report from your favorite broadcast.
https://mycroft.ai/skills
Apache License 2.0
9 stars 42 forks source link

Make less heavy assumptions on the rss format #13

Closed juhi24 closed 6 years ago

juhi24 commented 6 years ago

The code assumed the audio to be found in the first link of an rss entry. This is not the case for many news feeds (e.g. https://feeds.yle.fi/areena/v1/series/1-1440981.rss). Now it first sees if some of the links is an audio file. If not, it falls back to the previous functionality of picking the first link.

forslund commented 6 years ago

Hi, @juhi24 thank you for contributing this, sound like a good improvement and I'll review it in more detail during the day. In the mean time it would be good if you could sign our CLA.

To protect yourself, the project, and users of Mycroft technologies we require a Contributor Licensing Agreement (CLA) before accepting any code contribution. This agreement makes it crystal clear that along with your code you are offering a license to use it within the confines of this project. You retain ownership of the code, this is just a license.

Please visit https://mycroft.ai/cla to initiate this one-time signing. Thank you for contributing!

forslund commented 6 years ago

Works great! Will merge when the CLA arrives.

A Tip, you can use the python for-else structure to make the code a little cleaner. Something along the lines of:

            # After the intro, find and start the news stream
            # select the first link to an audio file
            for link in data['entries'][0]['links']:
                if 'audio' in link['type']:
                    media = link['href']
                    break
            else:
                # fall back to using the first link in the entry
                media = data['entries'][0]['links'][0]['href']
            url = re.sub('https', 'http', media)
juhi24 commented 6 years ago

Thanks for the quick review and the tip! I had the feeling there must be a more pythonic way than my original PR. If you have tested the for-each approach, please feel free to edit my PR. I think I checked the box allowing you to edit it.

My CLA is now in processing.

juhi24 commented 6 years ago

I have received and signed the CLA today.

forslund commented 6 years ago

Closing this one, I've moved the content to #19 and reopened against the 18.02 branch (new current default). Thanks for your awesome work!