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 newsfeed for the state of Hessen (Germany) #96

Open
wants to merge 4 commits into
base: 20.08
Choose a base branch
from
Open

Added newsfeed for the state of Hessen (Germany) #96

wants to merge 4 commits into from

Conversation

thorstenMueller
Copy link

As requested in issue #48 i've added a german newsfeed (radiostation) for the state of Hessen.

@krisgesling
Copy link
Contributor

Hey Thorsten,

With recent updates I've realised that we need to change the way we handle settings changes in the backend. Currently anytime a small change like this is made to a Skill, the backend treats it as a completely different block of settings. This means that it effectively wipes the settings for all existing users.

So I'm not keen to merge any more settings changes until we find a better way to handle this scenario. Unfortunately without the settings change, you would need to request the station by name each time.

A different way to get around this might be to update the default_stations function to provide more granular defaults? Eg anyone else in Hessen should get this by default rather than a more generic national station.

@thorstenMueller
Copy link
Author

Hey Kris.

This means that it effectively wipes the settings for all existing users.

Oh, didn't know about that.
Due i've currently no idea on how you could optimize your backend i wish you best of luck for finding a more suitable solution.

@thorstenMueller
Copy link
Author

Hey @krisgesling .
I've updated the PR based on your chat session.

What do you think?

Copy link
Contributor

@krisgesling krisgesling left a comment

Choose a reason for hiding this comment

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

Yeah, I think we can still include it without the settings change for the moment.

Thanks for cleaning up the intent_handler decorator too. We haven't really pushed the deprecation of it but the longer version is no longer needed and I think it will be less confusing for new devs if we just have the one.

__init__.py Outdated
@@ -119,6 +119,8 @@ def gbp():
'WDR': ('WDR', 'https://www1.wdr.de/mediathek/audio/'
'wdr-aktuell-news/wdr-aktuell-152.podcast',
image_path('WDR')),
'hr-iNFO': ('hr-iNFO', 'https://podcast.hr-online.de/der_tag_in_hessen/podcast.xml',
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the key could be shortened to HRI?
From memory this is only really used to match the Skill setting to the station. The voice commands are based on the first element of the tuple which would remain 'hr-info'

If there's other longer names that people might use to reference the station you can also add this into dialog/lang/alt.feed.name.value

Copy link
Author

Choose a reason for hiding this comment

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

I've updated PR based on your suggestions.

@krisgesling
Copy link
Contributor

Looks good.

"hr info" is a little hard to trigger as a station name but is the alias working well when you ask for the station by name?

@thorstenMueller
Copy link
Author

Hey @krisgesling
Sorry for the late response. I'll check this within the next days and give you feedback.

JarbasAl added a commit to OpenVoiceOS/ovos-skill-news that referenced this pull request Feb 14, 2023
JarbasAl added a commit to OpenVoiceOS/ovos-skill-news that referenced this pull request Feb 14, 2023
JarbasAl added a commit to OpenVoiceOS/ovos-skill-news that referenced this pull request Feb 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants