replace "|" in home assistant entity attributes with ":" in the output dialog

[PocketMiner82]

Description

If e.g. the friendly name in home assistant contains a "|", this would cause mycroft to randomly speak the text either before or after this character.

Type of PR

• [x] Bugfix

Testing

Note: I tested this with mycroft configured in german.

Rename the friendly name of an entity in home assistant to contain a "|" (for example "Battery | State of charge"). Then ask "Hey Mycroft, what is the state of battery state of charge"

Before, Mycroft would say something like: "State of Charge is ..." or just "Battery"

Now, it should say: "Battery : State of charge is ..."

[Tony763]

Hi, this happens in Mycroft-core function render in dialog.py.

https://github.com/MycroftAI/mycroft-core/blob/5f964f7d4036210c7e1977e6903d3f13421e2af2/mycroft/dialog/dialog.py#L111

@krisgesling Is this a wanted behavior of render?

Using | in entity name is not a good practice and can lead into troubles with others Home Assistant components/integrations.

If we want to add a command check later, we can't change entity name at input from API rather change it only for dialog.

Example:

def normalizedStr(input: str) -> str:
   """Replace unwanted symbols"""
    return input.replace("|", ":") # Or just empty string

....

self.speak_dialog('homeassistant.sensor', data={
                "dev_name": normalizedStr(sensor_name),
                "value": sensor_state,
                "unit": sensor_unit})

[stratus-ss]

@krisgesling going to tag you as per Tony's examination

[krisgesling]

Yeah this is intended behaviour of the dialog renderer so this change seems like a reasonable solution. Given no one's raised an issue so far let's merge it in.

[krisgesling]

Ah I just noticed we don't have the CLA bot running on here...

@PocketMiner82 I know it may seem like a small contribution however to protect yourself, the project, and users of Mycroft technologies we require a Contributor Licensing Agreement (CLA) before accepting any contributions to a Mycroft repository. This agreement makes it crystal clear that along with your code you are offering a license to use it within the confines of this project. You retain ownership of the code, this is just a license.

It might seem overkill on smaller PR's, however it greatly simplifies our processes to have a blanket rule for all contributions. I hope you understand.

On the plus side - you will also be added to our list of excellent human beings!

Please visit https://mycroft.ai/cla to initiate this one-time signing.

Thank you!

[Tony763]

Yeah this is intended behaviour of the dialog renderer so this change seems like a reasonable solution. Given no one's raised an issue so far let's merge it in.

Hi @krisgesling, I still do not think it's a good idea to change name directly in ha_client.py considering things I wrote above. If we want to normalize this, why not do it inside __init__.py?

Some like this would work for us without any future hassle:

# inside class
class HomeAssistantSkill(FallbackSkill):
....

    def _normalize_entity(entity: str) -> str:
        # Normalize entity name before speak

        entity = entity.replace("|", ":")
        return entity

....

self.speak_dialog('homeassistant.error.device.unknown', data={
                              "dev_name": _normalize_entity(entity)})

It will be a little more coding to write, but will have no down backs. Also for _normalize_entity() we can write unittest, so if more changes (in entity name) will be needed, we can check it still works.

[PocketMiner82]

Ah I just noticed we don't have the CLA bot running on here...

@PocketMiner82 I know it may seem like a small contribution however to protect yourself, the project, and users of Mycroft technologies we require a Contributor Licensing Agreement (CLA) before accepting any contributions to a Mycroft repository. This agreement makes it crystal clear that along with your code you are offering a license to use it within the confines of this project. You retain ownership of the code, this is just a license.

It might seem overkill on smaller PR's, however it greatly simplifies our processes to have a blanket rule for all contributions. I hope you understand.

On the plus side - you will also be added to our list of excellent human beings!

Please visit https://mycroft.ai/cla to initiate this one-time signing.

Thank you!

Hello. I didn't know that the signing was also required for skills. I've already read the agreement, but as I'm just a student, I don't want give my real name, address nor phone number. If this is really required, then probably someone else should create another pull request, maybe also with the suggestions of @Tony763 applied.

[Tony763]

Hi @PocketMiner82, that's not a problem. If You do not want to give these private information (I understood that), I can reauthor your PR with mention of Your nickname. Just say so. :slightly_smiling_face:

[PocketMiner82]

Hi @PocketMiner82, that's not a problem. If You do not want to give these private information (I understood that), I can reauthor your PR with mention of Your nickname. Just say so. :slightly_smiling_face:

Yes, if you want to, you can do this :)

[PocketMiner82]

Closed. Reason: New and better PR created by @Tony763 in #123