Skip to content
This repository has been archived by the owner on Sep 8, 2024. It is now read-only.

set active lang to configured if missing from STT and normalize #2650

Merged
merged 2 commits into from
Aug 10, 2020

Conversation

forslund
Copy link
Collaborator

@forslund forslund commented Aug 7, 2020

Description

I ran across this when checking some questions I had surrounding mycroft-timer PR #77

If the STT doesn't send a lang code fallback to configured language instead of hardcoded en-us. This should at least be more correct than the previous behaviour.

How to test

Check that Mycroft operates as usual and that the new unittest passes

Contributor license agreement signed?

CLA [ Yes ]

@devs-mycroft devs-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Aug 7, 2020
@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@forslund forslund force-pushed the bugfix/stt-missing-lang branch from 32dc0df to a5219a0 Compare August 7, 2020 12:53
@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@forslund forslund changed the title set active lang to configured if missing from STT set active lang to configured if missing from STT and normalize Aug 7, 2020
@krisgesling
Copy link
Contributor

Nice!

Is there going to be a benefit in re-using this elsewhere? ie should this be an imported util?

In terms of testing, is it worth us adding a test for a globally configured lang code?
Something like:

    def test_config_lang(self):
        msg = Message('test msg', data={})
        config = {'lang': 'it-it'}
        Configuration.patch(Message('patch', data={'config': config}))
        self.assertEqual(_get_message_lang(msg), 'it-it')
        Configuration.patch_clear(msg)

Sidenote - It seems like patch_clear doesn't really need any arguments, the Message arg looks like it's just been copy-pasted from the patch method. Will do a PR to propose removing that.

@krisgesling
Copy link
Contributor

disregard the patch_clear sidenote, just saw that it's got a messagebus handler so the Message arg makes total sense.

If the STT doesn't send a lang code fallback to configured language
instead of hardcoded en-us
This suites lingua franca better
@forslund forslund force-pushed the bugfix/stt-missing-lang branch from a5219a0 to e9760cd Compare August 10, 2020 05:42
@forslund
Copy link
Collaborator Author

I've added an explicit testcase for the case when the config doesn't contain a language code and changed it so the configured language code in the test is 'it-it' which should indeed be better than using the 'en-us' one which would be invisible.

I don't think it's of much use outside of the intentservice, the function could technically be placed in mycroft.util.lang, but since this is marked as "private" with the underscore, placing it here for now should be fine. If another use is found it can be added there and this can be removed.

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@krisgesling krisgesling merged commit 7d446b0 into MycroftAI:dev Aug 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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.

4 participants