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

Fix incorrect format call in case of missing breaking news update from ABC News Australia #106

Closed jaesharp closed 3 years ago

jaesharp commented 3 years ago

Description

We add some parameters to the URL formatting call which addresses a crashing bug in which the fallback case fails to properly format the string which, at some previous point required only a few parameters, now requires multiple parameters.

Type of PR

Testing

How can someone reviewing this PR test that it is working properly? Is there appropriate test coverage for this change?

A testing strategy should be defined for these modules, however this change fixes an immediate crashing bug in string formatting and as such does not change or add to existing functionality. This lack of testing must be addressed in future PRs to prevent a regression here.

krisgesling commented 3 years ago

G'day! Thanks for the fix and welcome to Mycroft!

Re ABC I've also noticed that they sometimes use different url formats :disappointed:

In case you run into that I did a quick PR to cycle through the formats I've seen: https://github.com/MycroftAI/skill-npr-news/pull/89

I just haven't annoyed anyone enough to review it yet, so it hasn't been merged... If you have any spare time, I would love to hear your thoughts :)

krisgesling commented 3 years ago

Also agree regarding the need for a testing strategy for these modules rather than relying solely on Voight Kampff integration tests.