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

Fix unused friendly name for SolarEdge sensor #20109

Merged
merged 1 commit into from
Jan 18, 2019

Conversation

LouisMT
Copy link
Contributor

@LouisMT LouisMT commented Jan 14, 2019

Description:

The SolarEdge component currently creates sensors named like SolarEdge_current_power in the front end, while an unused array containing friendly names exists in the code. This PR uses this array to set a friendly name.

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

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

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.

Breaking changes:

This PR changes the entity names of this sensor and the monitoring condition keys in the configuration:

Old entity name New entity name
sensor.solaredge_current_power sensor.solaredge_current_power
sensor.solaredge_last_day_data sensor.solaredge_energy_today
sensor.solaredge_last_month_data sensor.solaredge_energy_this_month
sensor.solaredge_last_year_data sensor.solaredge_energy_this_year
sensor.solaredge_life_time_data sensor.solaredge_lifetime_energy

@homeassistant
Copy link
Contributor

Hi @LouisMT,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@homeassistant homeassistant added platform: sensor.solaredge small-pr PRs with less than 30 lines. labels Jan 14, 2019
@ghost ghost added the in progress label Jan 14, 2019
@LouisMT
Copy link
Contributor Author

LouisMT commented Jan 14, 2019

I'll try to test this soon.

If user exposed functionality or configuration variables are added/changed

I think this will change the generated keys as well (sensor.solaredge_last_day_data -> sensor.solaredge_energy_today). Should this be noted in the documentation?

@MartinHjelmare
Copy link
Member

This is a breaking change. Please verify how the entity_id will change for all sensor types, and update the documentation as necessary.

@MartinHjelmare
Copy link
Member

Please add a paragraph in the PR description about the breaking change for the release notes.

@LouisMT
Copy link
Contributor Author

LouisMT commented Jan 16, 2019

To avoid confusion I've also changed the monitoring condition keys in the configuration to reflect the new sensor names. I've opened a PR for the documentation and noted breaking changes in this PR.

@MartinHjelmare
Copy link
Member

Great! Can be merged when build passes.

@pvizeli pvizeli merged commit e80702a into home-assistant:dev Jan 18, 2019
@ghost ghost removed the in progress label Jan 18, 2019
@balloob balloob mentioned this pull request Feb 6, 2019
alandtse pushed a commit to alandtse/home-assistant that referenced this pull request Feb 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants