Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add lektrico integration #102371

Merged
merged 55 commits into from
Aug 30, 2024
Merged

Add lektrico integration #102371

merged 55 commits into from
Aug 30, 2024

Conversation

Lektrico
Copy link
Contributor

@Lektrico Lektrico commented Oct 20, 2023

Proposed change

Add Lektrico integration

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

Additional information

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
  • I have followed the perfect PR recommendations
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

Copy link
Contributor

@Lash-L Lash-L left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a bit of feedback for you - I am not a core developer so feel free to ignore anything I say and wait for core feedback

homeassistant/components/lektrico/__init__.py Outdated Show resolved Hide resolved
Comment on lines 32 to 30
await coordinator.async_config_entry_first_refresh()

hass.data.setdefault(DOMAIN, {})[entry.entry_id] = coordinator
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably worth ensuring that coordinator.last_update_success is True. unless I am mistaken, UpdateFailed() being raise d in your coordinator will not interrupt this part of the flow

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, I added it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this another previous review fix which has been removed when rebasing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this another previous review fix which has been removed when rebasing?

No, I removed coordinator.last_update_success as suggested in Feb 16.

homeassistant/components/lektrico/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/lektrico/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/lektrico/manifest.json Outdated Show resolved Hide resolved
homeassistant/components/lektrico/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/lektrico/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/lektrico/sensor.py Outdated Show resolved Hide resolved
class LektricoSensorEntityDescription(SensorEntityDescription):
"""A class that describes the Lektrico sensor entities."""

value: Callable[[Any], float | str | int] | None = None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this optional none? You seem to always do it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your help.
I'm having problems with this proposed change:
If I replace
value: Callable[[Any], float | str | int] | None = None
with
value: Callable[[Any], float | str | int]
and I remove None from def native_value(self), I get this error:

2023-11-02 09:00:06.149 ERROR (MainThread) [homeassistant.loader] Unexpected exception importing platform homeassistant.components.lektrico.sensor
Traceback (most recent call last):
File "/workspaces/core/homeassistant/loader.py", line 836, in get_platform
cache[full_name] = self._import_platform(platform_name)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/workspaces/core/homeassistant/loader.py", line 853, in _import_platform
return importlib.import_module(f"{self.pkg_path}.{platform_name}")
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/local/lib/python3.11/importlib/init.py", line 126, in import_module
return _bootstrap._gcd_import(name[level:], package, level)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "", line 1204, in _gcd_import
File "", line 1176, in _find_and_load
File "", line 1147, in _find_and_load_unlocked
File "", line 690, in _load_unlocked
File "", line 940, in exec_module
File "", line 241, in _call_with_frames_removed
File "/workspaces/core/homeassistant/components/lektrico/sensor.py", line 36, in
@DataClass
^^^^^^^^^
File "/usr/local/lib/python3.11/dataclasses.py", line 1230, in dataclass
return wrap(cls)
^^^^^^^^^
File "/usr/local/lib/python3.11/dataclasses.py", line 1220, in wrap
return _process_class(cls, init, repr, eq, order, unsafe_hash,
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/local/lib/python3.11/dataclasses.py", line 1027, in _process_class
_init_fn(all_init_fields,
File "/usr/local/lib/python3.11/dataclasses.py", line 545, in _init_fn
raise TypeError(f'non-default argument {f.name!r} '
TypeError: non-default argument 'value' follows default argument
2023-11-02 09:00:06.164 ERROR (MainThread) [homeassistant.setup] Unable to prepare setup for platform lektrico.sensor: Platform not found (Exception importing homeassistant.components.lektrico.sensor).

Comment on lines 312 to 297
if self.entity_description.value is None:
return None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this sensor would just always be none?

Copy link
Contributor

@emontnemery emontnemery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some additional comments. I did not review sensor.py or the tests yet.

homeassistant/components/lektrico/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/lektrico/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/lektrico/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/lektrico/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/lektrico/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/lektrico/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/lektrico/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/lektrico/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/lektrico/coordinator.py Outdated Show resolved Hide resolved
homeassistant/components/lektrico/entity.py Outdated Show resolved Hide resolved
@home-assistant
Copy link

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.

@home-assistant home-assistant bot marked this pull request as draft February 16, 2024 14:08
@Lektrico Lektrico force-pushed the lektrico2 branch 5 times, most recently from 6de2599 to f5ec8ea Compare February 22, 2024 19:16
@Lektrico Lektrico marked this pull request as ready for review February 22, 2024 19:57
@home-assistant home-assistant bot requested a review from emontnemery February 22, 2024 19:57
homeassistant/components/lektrico/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/lektrico/coordinator.py Outdated Show resolved Hide resolved
homeassistant/components/lektrico/coordinator.py Outdated Show resolved Hide resolved
homeassistant/components/lektrico/coordinator.py Outdated Show resolved Hide resolved
homeassistant/components/lektrico/coordinator.py Outdated Show resolved Hide resolved
homeassistant/components/lektrico/coordinator.py Outdated Show resolved Hide resolved
homeassistant/components/lektrico/config_flow.py Outdated Show resolved Hide resolved
tests/components/lektrico/test_config_flow.py Outdated Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft March 4, 2024 16:35
@Lektrico Lektrico force-pushed the lektrico2 branch 2 times, most recently from 7532a4c to 0327374 Compare March 22, 2024 10:16
@Lektrico Lektrico force-pushed the lektrico2 branch 2 times, most recently from 38de2b5 to 55e193c Compare April 1, 2024 08:47
@Lektrico Lektrico marked this pull request as ready for review April 1, 2024 08:52
@joostlek joostlek merged commit 5bd7360 into home-assistant:dev Aug 30, 2024
40 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants