lolouk44 / CurrentCost_HA_CC

CurrentCost Meter Reading Custom Component for Home Assistant
16 stars 7 forks source link

Support `unique_id:` #19

Closed HarvsG closed 1 year ago

lolouk44 commented 1 year ago

Thanks a lot @HarvsG for your contribution. I no longer use my CurrentCost so can't test it. From a quick glance it looks ok. I take it you've fully tested this?

HarvsG commented 1 year ago

Not tested yet. I'll let you know when I have

HarvsG commented 1 year ago

tested on 2021.9 for annoying reasons but its working

before: image

after: image

HarvsG commented 1 year ago

Do you want me to put a version bump to 0.2.4 in the PR? And update the doc and changelog?

lolouk44 commented 1 year ago

Thanks for this. You can bump the version indeed and edit the change log if you want to help a bit more. Not sure the doc requires any update? I can then create a new release so it's picked up by HACS

HarvsG commented 1 year ago

Done

lolouk44 commented 1 year ago

Thanks. The developer documentation page states that the unique_id really shouldn't be set up in the config,

It is important that it is not possible for the user to change the unique ID, because the system would lose all its settings related to the unique ID.

I've not dealt with unique_ids in the past so don't have experience of setting them up, but could it not be set up in the sensor code? I'm not sure where you're located, I'm base in the UK (GMT+1 until end of month) and can be reached on discord under user lolouk44 #7180 if you want to discuss this?

HarvsG commented 1 year ago

I think that applies to integrations that are configured by config-flow:

PRs that add in configuration.yaml unique IDs are still being accepted https://github.com/home-assistant/core/pull/77461/files

And it is listed as YAML keys in a number of native integrations: https://www.home-assistant.io/integrations/template/#unique_id

lolouk44 commented 1 year ago

Thanks for coming back.

And it is listed as YAML keys in a number of native integrations: https://www.home-assistant.io/integrations/template/#unique_id

-> This applies to templateed sensors and is optional

PRs that add in configuration.yaml unique IDs are still being accepted https://github.com/home-assistant/core/pull/77461/files

-> This is in the sensor code, not in the config.yaml. I would suggest to do the same to avoid having the user come up with some sort of id. Can you replace unique_id = config.get(CONF_UNIQUE_ID) with unique_id = "mainsensorid"

Also looking at the pull example you provided, we also probably need to add a test to check whether the unique_id is indeed unique (i.e. does not already exist) e.g. something along the lines:

async def test_unique_id(hass):
    """Test unique_id property."""
    hass.states.async_set("sensor.test_sensor", "on")

    await async_setup_component(
        hass,
        "sensor",
        {
            "sensor": {
                "platform": "currentcost",
                "name": "currentcost",
                "unique_id": "mainsensorid",
            }
        },
    )
    await hass.async_block_till_done()
    er = entity_registry.async_get(hass)
    assert er.async_get("sensor.currentcost").unique_id == "mainsensorid"

What do you think? Is this something you're able to help with?