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.

Sign up for GitHub

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

Jump to bottom

Update to EnvoyReader 0.2, support for more hardware #16212

Merged
merged 8 commits into from
Sep 1, 2018
Merged

Update to EnvoyReader 0.2, support for more hardware #16212

merged 8 commits into from
Sep 1, 2018

Conversation

jesserizzo
Copy link
Contributor

@jesserizzo jesserizzo commented Aug 26, 2018 •
edited

Description:

Related issue (if applicable):
fixes #16210, fixes #16137

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#6088

Breaking Change: Change keys in monitored_conditions from 7_days_production and 7_days_consumption to seven_days_production and seven_days_consumption

Example entry for configuration.yaml (if applicable):

sensor:
  - platform: enphase_envoy
    ip_address: LOCAL_IP_FOR_ENVOY
    monitored_conditions:
      - production
      - consumption
      - lifetime_production
      - lifetime_consumption

Checklist:

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

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

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

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@ghost ghost added the in progress label Aug 26, 2018
@jesserizzo jesserizzo mentioned this pull request Aug 26, 2018
Merged
2 tasks
bachya
bachya previously requested changes Aug 27, 2018
View reviewed changes
Copy link
Contributor

@bachya bachya left a comment

Choose a reason for hiding this comment

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

Some small changes; additionally, please make sure you check off the appropriate boxes in the PR description as they are completed.

homeassistant/components/sensor/enphase_envoy.py Outdated
@@ -32,20 +32,20 @@
ICON = 'mdi:flash'

PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
vol.Required(CONF_IP_ADDRESS): cv.string,
vol.Optional(CONF_IP_ADDRESS, default="envoy"): cv.string,
Copy link
Contributor

Choose a reason for hiding this comment

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

Make "envoy" a constant and reference that here. Additionally, because of this, I assume documentation changes re in order?

homeassistant/components/sensor/enphase_envoy.py Outdated
@@ -81,27 +81,23 @@ def icon(self):

def update(self):
"""Get the energy production data from the Enphase Envoy."""
import envoy_reader
from envoy_reader import EnvoyReader

if self._type == "production":
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this wasn't the case originally, but since you're in here, these strings should be constants, as well.

MartinHjelmare
MartinHjelmare reviewed Aug 27, 2018
View reviewed changes
homeassistant/components/sensor/enphase_envoy.py Outdated
vol.Optional(CONF_MONITORED_CONDITIONS, default=list(SENSORS)):
vol.All(cv.ensure_list, [vol.In(list(SENSORS))])})


def setup_platform(hass, config, add_entities, discovery_info=None):
Copy link
Member

Choose a reason for hiding this comment

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

Revert change of add_entities. We've renamed that function.

homeassistant/components/sensor/enphase_envoy.py Outdated

if self._type == "production":
self._state = int(envoy_reader.production(self._ip_address))
self._state = EnvoyReader(self._ip_address).production()
Copy link
Member

Choose a reason for hiding this comment

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

If we add a map between sensor type and EnvoyReader method name, by extending SENSORS, we can simplify this code block significantly, by using getattr.

@MartinHjelmare
Copy link
Member

Changing the keys in SENSORS is a breaking change. Please add a paragraph in the PR description about this for the release notes.

@jesserizzo
Copy link
Contributor Author

jesserizzo commented Aug 28, 2018 •
edited

@MartinHjelmare
Maybe easier on users if I revert the change of the keys in SENSORS and just change the function names in EnvoyReader?
Nevermind, function names can't begin with a number. I'll add that change to the PR description.

@MartinHjelmare
Copy link
Member

You could add an item in the tuples of SENSORS with the method name, if you want to avoid a breaking change.

@jesserizzo
Copy link
Contributor Author

No, we might as just break it instead of making the code more complicated. Thanks for the ideas though.

MartinHjelmare
MartinHjelmare approved these changes Aug 28, 2018
View reviewed changes
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Looks good to me! @bachya what say you?

@fabaff fabaff dismissed bachya’s stale review September 1, 2018 21:45

Comments addressed

@fabaff fabaff merged commit 2b0b431 into home-assistant:dev Sep 1, 2018
@ghost ghost removed the in progress label Sep 1, 2018
girlpunk pushed a commit to girlpunk/home-assistant that referenced this pull request Sep 4, 2018
@jesserizzo
Update to EnvoyReader 0.2, support for more hardware (home-assistant#…
1302961 
…16212)

* Add support for older Envoy models

* Stop requiring envoy model name in config

* Update to envoy_reader0.2

* Minor formatting fixes

* run script/gen_requirements_all.py

* Minor formatting fixes

* Change some strings to constants, use getattr to call function
@balloob balloob mentioned this pull request Sep 17, 2018
Merged
@home-assistant home-assistant locked and limited conversation to collaborators Dec 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MartinHjelmare MartinHjelmare MartinHjelmare approved these changes

@bachya bachya bachya left review comments

Assignees
No one assigned
Labels
breaking-change cla-signed integration: enphase_envoy small-pr PRs with less than 30 lines.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enphase Envoy Component not working on envoy R4.10.24 Enphase Envoy Component
5 participants
@jesserizzo @MartinHjelmare @bachya @fabaff @homeassistant