search

MycroftAI / skill-weather

Mycroft AI official Weather Skill, providing weather conditions and forecasts.
https://mycroft.ai/skills
Apache License 2.0
19 stars 59 forks source link

Prep for future update: don't pass 'lang=None' to LF functions #140

Closed ChanceNCounter closed 3 years ago

ChanceNCounter commented 4 years ago

I'd fix it myself, if I didn't have so much going on. I've found one breaking change in the pending Lingua Franca refactor, which is this: explicitly passing lang=None to parsers and formatters will get angry.

I'm comfortable accounting for this in LF, by overriding None with the current default. But, if we do that, I'd still vote to slap a big ol' DeprecationWarning on it, because passing lang=None is kinda paradigm-defying. I think it was a (perfectly rational) kludge in the past.

(Note: explicitly passing lang='en_us' where the skill hasn't been localized is fine, but something's gonna need to happen (in skills or in core) to ensure that English is currently loaded. Otherwise, an exception will be raised complaining that English is not currently loaded. That's out of scope for this issue, except that those lang='en-us' calls don't need to be changed. Just the lang=None ones.

krisgesling commented 4 years ago

Yeah, seems weird, we should definitely handle that better here, not need to override a seemingly intentionally passed variable elsewhere.

forslund commented 4 years ago

For a bit of history. It was in the beginning required to provide a language code, then a change was added to set the default lang code based on system config.

Mainly to spare skill creators from typing lang=self.lang to all translate methods. I think this may still be a good thing to keep in the mycroft-core wrapper to LF

It's not a bug it's a feature .)

krisgesling commented 3 years ago

I believe these have all been removed in the new version so closing.