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

Issue 2120 - Fallback to 'en-us' when resource loading fails #2392

Closed
wants to merge 4 commits into from

Conversation

domcross
Copy link
Contributor

@domcross domcross commented Nov 22, 2019

Description

When mycroft_skill.find_resource fails to load a resource for self.lang fall back to lang 'en-us' as most skills/resources are available in english by default.

==== Fixed Issues ====
#2120

How to test

There are several skills missing some intent-files for 'de-de' (among them is the mycroft-weather skill). Without this PR loading of these skills will fail. When PR is applied all skills will load, but some WARNINGs will be logged.

Contributor license agreement signed?

CLA [X] (Whether you have signed a CLA - Contributor Licensing Agreement

When mycroft_skill.find_resource fails to load a resource for self.lang
fall back to lang 'en-us'
as most skills/resources are available in english by default.

==== Fixed Issues ====
MycroftAI#2120
When mycroft_skill.find_resource fails to load a resource for self.lang
fall back to lang 'en-us'
as most skills/resources are available in english by default.

==== Fixed Issues ====
MycroftAI#2120
@devs-mycroft devs-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Nov 22, 2019
@domcross domcross changed the title Issue 2120 Issue 2120 - Fallback to 'en-us' when resource loading fails Nov 22, 2019
Copy link
Collaborator

@forslund forslund left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this,

just had a minor nitpick (see comment in code) otherwise it looks very good and mergable.

@@ -675,9 +675,22 @@ def find_resource(self, res_name, res_dirname=None):
Returns:
string: The full path to the resource file or None if not found
"""
result = self._find_resource(res_name, self.lang, res_dirname)
if not result:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add and self.lang != 'en-us' to this if-statement so it doesn't try to get English resources twice if the file is missing.

@forslund
Copy link
Collaborator

forslund commented Dec 3, 2019

Tested some more and this doesn't handle dialogs but that might be outside the scope of the PR?

@domcross
Copy link
Contributor Author

domcross commented Dec 3, 2019

Yes, dialogs arewere not handled by this. Main scope of this PR is that skill loading does not fail in case a intent file is missing.
I had a quick look at the DialogRender class, so here we go ;-)

@forslund
Copy link
Collaborator

forslund commented Dec 5, 2019

I merged the commits needed to load the skills through #2411, I had some issues with the dialogs so we might need to go over those again. Thanks for contributing this and I hope the German experience improves (I know my Swedish experience will!)

If you like to continue the work on dialog and .voc files we can discuss the issues further. In the mean time I'm closing this PR and you/we can move the last commit over to a new branch based on the latest dev branch to continue the work.

Once more, many thanks.

@forslund forslund closed this Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants