Style change follow up by cdce8p · Pull Request #13707 · home-assistant/core

cdce8p · 2018-04-05T20:30:22Z

Follow up to: #13654

Description:

During the last style change @kellerza suggested to rework what is passed into the HomeAccessory constructor. I think to following changes might make sense:

hass is now a class variable of HomeAccessory and HomeBridge and set during the HomeKit setup. That way it doesn't need to be passed to each accessory individually.
For the HomeAccessory I added the method update_state_callback which checks that the new state isn't None (previously done in each type) and calls the update_state method.

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass
Tests have been added to verify that the new code works.

balloob · 2018-04-06T19:49:14Z

We should not do this.

All of Home Assistant is coded in a way that binds all gobal state to the hass object. Class variables go against this. Pass it into the constructor or set it as an instance variable. Don't set it as a class variable.

kellerza · 2018-04-07T21:59:39Z

This signature looks much better, if you add hass here, it should adress @balloob's concerns as well.

Also means no change required in __init__.py

kellerza · 2018-04-07T22:05:48Z

👍 This is a good idea and seem to simplify things as well

kellerza · 2018-04-07T22:12:17Z

Also leave hass in here, and try to keep same signature as __init__ in HomeAccessory (at the moment you swap name & entity_id).

Maybe keep name first, since Accessory's signature is (name, **kwargs)

*same for other HomeAccesories

kellerza · 2018-04-07T22:20:43Z

If you call category with a keyword it also protects against future changes

super().__init__(name, entity_id, category=CATEGORY_*, **kwargs)

*same for all HomeAccessories

kellerza · 2018-04-07T22:36:06Z

Once all Accesory signature are the same you can simplify this:

def get_accessory(hass, state, aid, config): """Take state and return an accessory object if supported.""" if not aid: _LOGGER.warning('The entitiy "%s" is not supported, since it ' 'generates an invalid aid, please change it.', state.entity_id) return None a_type = None a_kwargs = {'aid'} = aid if state.domain == 'sensor': unit = state.attributes.get(ATTR_UNIT_OF_MEASUREMENT) if unit in (TEMP_CELSIUS, TEMP_FAHRENHEIT): a_type = 'TemperatureSensor' elif unit == '%': a_type = 'HumiditySensor' elif state.domain == 'cover': # Only add covers that support set_cover_position features = state.attributes.get(ATTR_SUPPORTED_FEATURES, 0) if features & SUPPORT_SET_POSITION: a_type = 'WindowCovering' elif state.domain == 'alarm_control_panel': a_type = 'SecuritySystem' a_kwargs['alarm_code'] = config.get(ATTR_CODE) elif state.domain == 'climate': features = state.attributes.get(ATTR_SUPPORTED_FEATURES, 0) support_temp_range = SUPPORT_TARGET_TEMPERATURE_LOW | \ SUPPORT_TARGET_TEMPERATURE_HIGH # Check if climate device supports auto mode a_kwargs['support_auto'] = bool(features & support_temp_range) a_type = 'Thermostat' elif state.domain == 'light': a_type = 'Light' elif state.domain in ('switch', 'remote', 'input_boolean', 'script'): a_type = 'Switch' if a_type is None: return None _LOGGER.debug('Add "%s" as "%s"', state.entity_id, state.name) return TYPES[a_type](hass, state.name, state.entity_id, **a_kwargs)

cdce8p · 2018-04-08T22:00:49Z

Thanks for the feedback. I wasn't home this weekend, so I haven't had the chance to do the improvements yet. Hopefully during the next few days, although I have already done a rebase.

* Set argument order for HomeAccessory and HomeBridge * Added update_state_callback

cdce8p · 2018-04-09T22:35:25Z

I have update the commits to reverse the change to make hass a static var and incorporated the over feedback. Especially the change of get_accessory as suggested by @kellerza.

kellerza · 2018-04-09T22:53:07Z

                        'generates an invalid aid, please change it.',
                        state.entity_id)
-        return None
+        return


Although they are the same, I would suggest we keep return None where we care / intend to use the return of a function

I have again added it to get_accessory and generate_aid. In cases where it is an abort condition (like HomeKit.start(), HomeKit.stop(), HomeAccessory.update_state_callback()) I would prefer to stay with just return.

cdce8p · 2018-04-10T05:59:49Z

-        self.hass = hass
-        self.entity_id = entity_id
+        super().__init__(hass, name, entity_id,
+                         category=CATEGORY_WINDOW_COVERING, **kwargs)


Personally I would really like to move these calls to the HomeAccessory class somehow. Any ideas who to improve that?

On solution could be to use a decorator function for for the init method which calls super().__init__(). That would look like this:

def super_init(category): def category_wrapper(func_init): def wrapper(self, hass, name, entity_id, **kwargs): super(self.__class__, self).__init__( hass, name, entity_id, category=CATEGORY_LIGHT, **kwargs) func_init(self) return wrapper return category_wrapper @TYPES.register('Light') class Light(HomeAccessory): @super_init(CATEGORY_LIGHT) def __init__(self):

Should we do that or stay at the current:

def __init__(self, hass, name, entity_id, **kwargs): super().__init__(hass, name, entity_id, category=CATEGORY_LIGHT, **kwargs)

I don't like the decorator and not sure how that will interfere with __init__

You could do this (note: category= at the end so they look the same at first glance)

def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs, category=CATEGORY_LIGHT)

but being explicit might give you some code editor hints, so I like this version more:

def __init__(self, hass, name, entity_id, **kwargs): super().__init__(hass, name, entity_id, **kwargs, category=CATEGORY_LIGHT)

The idea behind the decorator was to limit code redundancy between different types. You're right the it might be more difficult for editors to read, however I don't think that is that big of a deal since those attributes are already assigned in accessories.py/HomeAccessory and most inexperience developers will only copy existing types without fully understanding them.

Regarding the interference with __init__, it should change anything. The call order is as follows:

HA start

Module is loaded and __init__ call is replaced by decorater

Call to light.__init__

super().__init__()

__init__()

If you dont like the super rather add a bootstrap/postinit method in the child classes that you call from the parent’s init. Type could be a class var on the child

In HomeAccessory you then have:

def __init__(self, hass, name, entity_id, **kwargs): """Initialize a Accessory object.""" super().__init__(name, **kwargs) set_accessory_info(self, name, model=entity_id) self.entity_id = entity_id self.hass = hass cat = self.init_category() self.category = getattr(Category, cat, Category.OTHER)

with only def init_category(self) and no more __init__ in the rest, init_category then returns the category.

Extending on your idea what do you think about the following:

Making the category a static var of the type

Changing the __init__() of the types to something like init_setup() which is called from HomeAccessory.__init__()

init_setup() might not be the best name. If you have something else in mind, I'm happy to change it.
The only caveat with this solution is, that I had to add # pylint: disable=attribute-defined-outside-init to every type file.

cdce8p · 2018-04-10T20:16:43Z

@kellerza What do you think about a helper function to create characteristics? I thought about something like this (example from type_lights, char_on, code):

# Class HomeAccessory

    def setup_char(self, service, char_name, char_var_name,
                   value=None, callback=None):
        char = self.__dict__[char_var_name] = \
            service.get_characteristic(char_name)
        if value:
            char.value = value
        if callback:
            char.setter_callback = callback

# Type lights (init / init_setup)

        self.setup_char(serv_light, CHAR_ON, 'char_on',
                        value=self._state, callback=self.set_state)

kellerza

From the code trying to remove super().__init__(x) is forced.

Rather just stick to

    super().__init__(*args)

From you changes it does not seem like **kwargs is required any longer, only aid

kellerza · 2018-04-10T21:46:25Z

        a_type = 'Switch'

-    if a_type is None:
+    else:


This change will cause issues....
state.domain == 'sensor' and unit is not %, or temp and a_type is undefined

Please revert to the original if a_type is None:

You're right. Didn't thought about that.

kellerza · 2018-04-10T21:47:30Z

        return None

-    a_type = None
-    a_kwargs = {'aid': aid}


Keep a_type = None and config = {}

kellerza · 2018-04-10T21:50:15Z

-        return TYPES['Switch'](hass, state.entity_id, state.name, aid=aid)
+        a_type = 'Switch'
+
+    else:


if a_type is None:

kellerza · 2018-04-10T21:51:33Z

-                          state.entity_id, 'HumiditySensor')
-            return TYPES['HumiditySensor'](hass, state.entity_id, state.name,
-                                           aid=aid)
+    if state.domain == 'alarm_control_panel':


Insert a_type = None

kellerza · 2018-04-10T21:52:35Z

@@ -1,4 +1,5 @@
 """Class to hold all cover accessories."""
+# pylint: disable=attribute-defined-outside-init
