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

Fix dialog variable names in non-english languages #182

Closed Joanguitar closed 3 years ago

Joanguitar commented 3 years ago

Description

On top of my previous PR, I want to fix all the old language formatting that is not working anymore. Things like changing {temp} to {temperature}. I included the script I used to automate this job just in case you want to review it but it's not there to stay.

Type of PR

Bugfix

Testing

Just test this skill in any of the languages that were using the old format, such as: es-es, ca-es, da-dk, de-de, fr-fr, gl-es, and the list just keeps going.

Documentation

Nothing new, just a bugfix

Joanguitar commented 3 years ago

I don't see the overkill here, the language is not updated unless there's a change to it, I don't think a string comparison adds an unreasonable overhead for multi-language support of the skill. On the other hand, modifying the core of Mycroft forcing skill to react to changes in the languages seems tedious for future skill developers. The idea is to have this skill react to changes in the language which is an already defined variable (self.lang) in Mycroft skills. I'm just trying to anticipate to multi-language support. I already have a working version of Mycroft with multi-language support for the intent service and this skill was the only one that was not reacting to changes in the skill language among the ones I tested.

chrisveilleux commented 3 years ago

While I appreciate your efforts to improve language support in Mycroft, this change does not align with Mycroft's event-driven architecture. Checking for a language change every time the user asks for the weather when a (rare) change to the configuration value can be detected and communicated across the bus is less than ideal.

I am curious about what you find tedious in emitting and reacting to events on the message bus. Only those skills that care about the event will consume it. This solution also facilitates having a single place in the code that checks for language changes, making it easier debug potential language support issues.

Joanguitar commented 3 years ago

The idea was to let the skill react to the language of the message that triggered it such as already implemented in HolmesV. Skills are supposed to use the variable self.lang to create the response, such as other skills already do. To put an example of this using other Mycroft official skills: joke skill, timer-skill date-time-skill wiki-skill. All these skills already support multi-language because they don't hard-code the language of the skill during the initialization.

It would be very tedious in the future if every time a message is sent, an event has to be emitted and every skill has to react to it. Even more tedious for skill developers to have to consider these events instead of reading the language the have to work with from the already implemented self.lang.

I really hope that a string comparison doesn't get in the way of multi-language support because "it is an overkill".

Joanguitar commented 3 years ago

I implemented the changes suggested in the Mycroft community chat by username chance, now the API doesn't store its own language during the initialization and makes use of self.lang instead.

krisgesling commented 3 years ago

Thanks Joan - very much appreciate the cleanup.

I've deleted the script used to create it since it's not needed for the Skill but a similar approach will be useful for any other Skills wanting to change variable names in dialog files.