Use feed name as entity name in GeoJSON

[codyc1515]

Breaking change

Previously GeoJSON names were just the config entry ID. This was not very user friendly. Particularly so when there are many config entries and many, many entities from those same many config entries. This is how they look today.

Proposed change

The proposal is to add support for entity name in GeoJSON. If unavailable in the data source, falls back to the config entry (same as today). This is how they look after.

Type of change

• Dependency upgrade
• Bugfix (non-breaking change which fixes an issue)
• New integration (thank you!)
• New feature (which adds functionality to an existing integration)
• Deprecation (breaking change to happen in the future)
• Breaking change (fix/feature causing existing functionality to break)
• Code quality improvements to existing code or addition of tests

Checklist

• The code change is tested and works locally.
• Local tests pass. Your PR cannot be merged unless tests pass
• There is no commented out code in this PR.
• I have followed the [development checklist][dev-checklist]
• I have followed the [perfect PR recommendations][perfect-pr]
• The code has been formatted using Ruff (ruff format homeassistant tests)
• Tests have been added to verify that the new code works.

[home-assistant]

Hey there @exxamalte, mind taking a look at this pull request as it has been labeled with an integration (geo_json_events) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of geo_json_events can trigger bot actions by commenting:

• @home-assistant close Closes the pull request.
• @home-assistant rename Awesome new title Renames the pull request.
• @home-assistant reopen Reopen the pull request.
• @home-assistant unassign geo_json_events Removes the current integration label and assignees on the pull request, add the integration domain after the command.
• @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
• @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

[codyc1515]

Looks like the tests need updating to support this. I have no idea how to do that, sorry.

[exxamalte]

Hm, I'm not sure if we're heading into the right direction here. Picking a random property and making it into the entity's name might look like a good idea, but there could be hundreds of feeds out there that don't define that particular property. The GeoJSON specification provides absolutely no schema for the properties.
Do you use many GeoJSON feeds that do have a "name" property?

[codyc1515]

We aren't picking a random property - we're picking one specifically, called name.

The official GeoJSON website uses it in their minimalist example. LeafletJS, which HA uses for Maps, also endorses this usage. This is also how ArcGIS uses the properties. In fact, I've only ever seen GeoJSON used in the way I've described.

If we didn't find a name, we fallback to the previous case (the config_entry id). So, the change should not "break" for most people.

[exxamalte]

Looks like the tests need updating to support this. I have no idea how to do that, sorry.

The tests appear to fail because the geo_location entities couldn't be added because there is something wrong with their names (or at least how the test is set up to generate names). The root cause appears to be:

ERROR:homeassistant.components.geo_location:Error adding entities for domain geo_location with platform geo_json_events
Traceback (most recent call last):
  File "/home/runner/work/core/core/homeassistant/helpers/entity_platform.py", line 515, in async_add_entities
    await asyncio.gather(*tasks)
  File "/home/runner/work/core/core/homeassistant/helpers/entity_platform.py", line 666, in _async_add_entity
    entry = entity_registry.async_get_or_create(
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/runner/work/core/core/homeassistant/helpers/entity_registry.py", line 695, in async_get_or_create
    entity_id = self.async_generate_entity_id(
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/runner/work/core/core/homeassistant/helpers/entity_registry.py", line 615, in async_generate_entity_id
    preferred_string = f"{domain}.{slugify(suggested_object_id)}"
                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/runner/work/core/core/homeassistant/util/__init__.py", line 47, in slugify
    slug = unicode_slug.slugify(text, separator=separator)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/runner/work/core/core/venv/lib/python3.11/site-packages/slugify/slugify.py", line 95, in slugify
    text = str(text, 'utf-8', 'ignore')
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: decoding to str: need a bytes-like object, MagicMock found

I think if you fix the linter error, this might just go away.

Nevertheless it would be nice to add a test case to see if fetching the "name" property actually works.

[codyc1515]

The linter warning is saying that the code looks pointless where we check if we can use the entity name or need to fall back to the config entry id or not.

[codyc1515]

Looks like I broke the CI - it's just sitting on Waiting to run this check. Appreciate any assistance with resolving the test. It looks like the test is failing on the slugify which is due to something to do with the entity name not populating (for the purposes of the test only, to be clear). The integration itself is actually working fine with this change.

[codyc1515]

Hm, I'm not sure if we're heading into the right direction here. Picking a random property and making it into the entity's name might look like a good idea, but there could be hundreds of feeds out there that don't define that particular property. The GeoJSON specification provides absolutely no schema for the properties. Do you use many GeoJSON feeds that do have a "name" property?

I'm wondering now if we should actually make this a config parameter somewhere. Still default it to name or fallback to the config_entry id but also give the option of a textbox through the config flow to choose another different key to use as the title, such as Address (as in street address). This could be configured to maybe only be an Advanced Users parameter.

[joostlek]

Tests are failing, can you take a look

[home-assistant]

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

[codyc1515]

Tests are failing, can you take a look

Sorry, as above, I don't know how to fix that. The changes themselves work, simply the tests need to be updated.

[codyc1515]

When I replace your change with the second code snippet I recommended to try, the unit tests pass on my machine without any modifications.

Sorry, I did try that and thought that it was failing. I must have tested it with a wrong feed (one that doesn't have a name propety). Can confirm it is working and the tests passed now :) Thank you very much for your help.

[codyc1515]

I will update the documentation before we push this.

[codyc1515]

I have added added tests to cover the new scenarios too. All tests passed.

[exxamalte]

This looks good to me. Thanks for adding the test case.

[joostlek]

Oh btw, a name change isn't a breaking change

[codyc1515]

Thanks. I wasn't sure, so went conservative.

[MartinHjelmare]

Please address the comment in a new PR, if needed. Thanks