architecture icon indicating copy to clipboard operation
architecture copied to clipboard

Checks for non-implemented methods

Open ties opened this issue 6 years ago • 2 comments

Context

While refactoring the climate component, unimplemented methods were missed (e.g. https://github.com/home-assistant/home-assistant/issues/25239).

This was not caught before release, likely because (a) unit tests for the platform were not ran on the implementation and (b) the abstract-method pylint check was disabled, likely due to false positives.

Proposal

Re-enable the abstract-method check for pylint. If this causes issues (with subclasses that are still abstract), proceed with the alternative.

Alternative:

Use python abstract base classes to create abstract methods using abc.abstractmethod instead of raising NotImplementedError. Using this, an implementation with missing methods can not be instantiated.

With Entity a subclass of ABC, @abc.abstratmethod should be usable in the implementation of the platforms.

Consequences

Implementations with missing methods will be broken, causing tests to fail that currently pass. Currently, the tests would not touch the missing method.

In addition, using abstract base classes implies the usage of a certain metaclass* type, potentially creating "metaclass conflicts" with multiple-inheritance when another class uses a different metaclass. This could be an advantage: I think using custom metaclasses is complex and use cases are very limited.

*: approximately: "A class that defines the behaviour of other classes"

ties avatar Jul 18 '19 14:07 ties

Based on the supported features of a device, certain methods don't have to be implemented. We raise so that it's clear that it's a bug and it won't be silently continuing.

balloob avatar Jul 18 '19 20:07 balloob

With the abstract base class, you would get the error once a object is instantiated. This is very likely to happen during unit tests. Which I think is better than getting it after a release.

You have two main situations:

  • Methods exist on type, are not called due to state of object/contract design of API, no bugs
  • Methods do not exist on type, are called, NotImplementedError from base class, bug

My experience that it is hardly feasible (very high effort) to make sure that a PR implements all required methods, so would leave this to linter/code if possible.

ties avatar Jul 19 '19 11:07 ties

This architecture issue is old, stale, and possibly obsolete. Things changed a lot over the years. Additionally, we have been moving to discussions for these architectural discussions.

For that reason, I'm going to close this issue.

../Frenck

frenck avatar May 11 '23 13:05 frenck