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

Add Solar-Log platform #27036

Merged
merged 31 commits into from Oct 23, 2019

Conversation

@Ernst79
Copy link
Contributor

Ernst79 commented Sep 28, 2019

Description:

Adding Solar-Log platform for Solar-Log (PV monitoring) devices.

Example entry for configuration.yaml (if applicable):

sensor:
platform: solarlog

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.
  • I have followed the [development checklist][dev-checklist]

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

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

  • [The manifest file][manifest-docs] has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.
@homeassistant

This comment has been minimized.

Copy link
Contributor

homeassistant commented Sep 28, 2019

Hi @Ernst79,

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!

@project-bot project-bot bot added this to Needs review in Dev Sep 28, 2019
@Ernst79 Ernst79 referenced this pull request Sep 28, 2019
2 of 2 tasks complete
@homeassistant homeassistant added cla-signed and removed cla-needed labels Sep 28, 2019
script/gen_requirements_all.py Outdated Show resolved Hide resolved
@balloob

This comment has been minimized.

Copy link
Member

balloob commented Oct 4, 2019

Hey Ernst, thanks for your first contribution. Welcome to the Home Assistant community! 🐬

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Oct 4, 2019

May I suggest that you use a config flow instead of platform configuration? You can do it as follows:

You can do it easily with our new scaffolding tool. From your dev environment, run:

python3 -m script.scaffold config_flow

It will generate a config flow and tests that pretty much match your schema 👍 Then change setup_platform into async_setup_entry.

Co-Authored-By: Paulus Schoutsen <paulus@home-assistant.io>
Ernst79 added 4 commits Oct 5, 2019
I've set it to a fixed scan_interval of 1 minute. Removed the configuration option.
@Ernst79

This comment has been minimized.

Copy link
Contributor Author

Ernst79 commented Oct 5, 2019

May I suggest that you use a config flow instead of platform configuration? You can do it as follows:

You can do it easily with our new scaffolding tool. From your dev environment, run:

python3 -m script.scaffold config_flow

It will generate a config flow and tests that pretty much match your schema 👍 Then change setup_platform into async_setup_entry.

Good suggestion. I do like to make this change in the future, but as I'm not a programmer, I need time to look into this and understand what I'm doing.

@Ernst79 Ernst79 requested a review from balloob Oct 5, 2019
@frenck frenck removed the docs-missing label Oct 12, 2019
Ernst79 added 5 commits Oct 17, 2019
@balloob

This comment has been minimized.

Copy link
Member

balloob commented Oct 18, 2019

Resolving the conflict was easy, I just pressed the "resolve conflict" button for you 👍

Did you run python3 -m script.scaffold config_flow ? It looks like you have diverted quite a bit from the original template, including the tests are missing.

Ernst79 added 3 commits Oct 18, 2019
@Ernst79

This comment has been minimized.

Copy link
Contributor Author

Ernst79 commented Oct 18, 2019

@balloob I've added the tests. Yes, I've differted a bit from the template, as I like to be able to use the yaml config as well. I've used the cert_expiry sensor as a template, moved a part of sensor.py to const.py.

Ernst79 added 9 commits Oct 20, 2019
@Ernst79 Ernst79 requested a review from balloob Oct 21, 2019
@Ernst79

This comment has been minimized.

Copy link
Contributor Author

Ernst79 commented Oct 22, 2019

@balloob All remarks should be resolved now. Also updated the documentation. Only the remark about query-ing the name from the Solar-Log connection is not possible, I think, see my comment on that question above.

Copy link
Member

balloob left a comment

Awesome! 🎉 Great first contribution 🐬 Welcome to the Home Assistant community 👋

Dev automation moved this from Needs review to Reviewer approved Oct 23, 2019
@balloob balloob merged commit acc3646 into home-assistant:dev Oct 23, 2019
11 checks passed
11 checks passed
CI Build #20191021.53 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch Coverage not affected when comparing 4893401...3f82f44
Details
codecov/project 94.33% (target 90%)
Details
Dev automation moved this from Reviewer approved to Done Oct 23, 2019
@lock lock bot locked and limited conversation to collaborators Oct 24, 2019
@Ernst79 Ernst79 deleted the Ernst79:solarlog branch Oct 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
4 participants
You can’t perform that action at this time.