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

Minor style changes, cleanup #13654

Merged
merged 2 commits into from
Apr 4, 2018
Merged

Minor style changes, cleanup #13654

merged 2 commits into from
Apr 4, 2018

Conversation

cdce8p
Copy link
Member

@cdce8p cdce8p commented Apr 3, 2018

Description:

Extracted from #13534, to get merged quicker for #13625

  • Change self._entity.id to self.entity_id
  • Use const STATE_OFF
  • Added CATEGORY constants
  • Removed *args from accessory types
  • Changed self._hass to self.hass

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

* Change 'self._entity.id' to 'self.entity_id'
* Use const 'STATE_OFF'
* Added CATEGORY constants
* Removed *args from accessory types
* Changed 'self._hass' to 'self.hass'
@@ -65,10 +65,10 @@ def _set_services(self):

def run(self):
"""Method called by accessory after driver is started."""
state = self._hass.states.get(self._entity_id)
state = self.hass.states.get(self.entity_id)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe pass hass and entity_id into the HomeAccesory constructor, then you can remove those assignments from all child classes & you dont need the no-member pylint directive

"""Initialize a new Light accessory object."""
super().__init__(name, entity_id, CATEGORY_LIGHT, *args, **kwargs)
super().__init__(name, entity_id, CATEGORY_LIGHT, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

Your parent class constructor signature does not have entity_id, but you seem to include it here?

def __init__(self, name=ACCESSORY_NAME, model=ACCESSORY_MODEL,
           category='OTHER', **kwargs):

@cdce8p
Copy link
Member Author

cdce8p commented Apr 3, 2018

@kellerza I know what you mean. Currently the model is set to the entity_id. So technical I already do that, I just don't use it.

However, I wouldn't want to change that at the moment, since I don't instantiate a HomeAccessory object directly and we are currently changing / improving HAP-python quite a bit. Therefore it will likely change in future anyway.

* For added lights
@cdce8p cdce8p mentioned this pull request Apr 4, 2018
3 tasks
@cdce8p cdce8p merged commit 692b264 into home-assistant:dev Apr 4, 2018
@cdce8p cdce8p deleted the homekit-style-changes branch April 4, 2018 22:52
@kellerza
Copy link
Member

kellerza commented Apr 4, 2018

Even if you update HAP-Python, this design is still questionable!!

But hey, don’t mind the reviewers...

@cdce8p
Copy link
Member Author

cdce8p commented Apr 4, 2018

I'm sorry I merged this, but there are three other PR's depending on this and it will be addressed later.

@cdce8p cdce8p mentioned this pull request Apr 5, 2018
3 tasks
@balloob balloob mentioned this pull request Apr 13, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Jul 26, 2018
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.

3 participants