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

New EDP re:dy component #16426

Merged
merged 15 commits into from
Sep 15, 2018
Merged

New EDP re:dy component #16426

merged 15 commits into from
Sep 15, 2018

Conversation

abmantis
Copy link
Contributor

@abmantis abmantis commented Sep 4, 2018 •
edited
Loading

Description:

New integration of the EDP re:dy platform.

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

Example entry for configuration.yaml (if applicable):

edp_redy:
  username: YOUR_USERNAME
  password: YOUR_PASSWORD

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox.

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

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

  • New files were added to .coveragerc.

@ghost ghost added the in progress label Sep 4, 2018
@abmantis abmantis changed the title new EDP re:dy component New EDP re:dy component Sep 4, 2018
@abmantis abmantis mentioned this pull request Sep 4, 2018
Merged
2 tasks
MartinHjelmare
MartinHjelmare requested changes Sep 5, 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.

The EdpRedySession class should be extracted.

homeassistant/components/edp_redy.py Outdated Show resolved Hide resolved
houndci-bot
houndci-bot reviewed Sep 6, 2018
View reviewed changes
homeassistant/components/edp_redy.py Outdated
from edp_redy import EdpRedySession

session = EdpRedySession(config[DOMAIN][CONF_USERNAME],
config[DOMAIN][CONF_PASSWORD])
Copy link

Choose a reason for hiding this comment

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

continuation line over-indented for visual indent

@abmantis
Copy link
Contributor Author

abmantis commented Sep 7, 2018

By the way, is the following lint issue a false positive?
homeassistant/components/edp_redy.py:57:19: E0601: Using variable 'platform_loaded' before assignment (used-before-assignment)

MartinHjelmare
MartinHjelmare reviewed Sep 8, 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.

Please follow our style guide for quotes. There are some inconsistencies.

https://developers.home-assistant.io/docs/en/development_guidelines.html#quotes

homeassistant/components/sensor/edp_redy.py Outdated Show resolved Hide resolved
homeassistant/components/sensor/edp_redy.py Outdated Show resolved Hide resolved
homeassistant/components/sensor/edp_redy.py Outdated Show resolved Hide resolved
homeassistant/components/sensor/edp_redy.py Outdated Show resolved Hide resolved
homeassistant/components/sensor/edp_redy.py Outdated Show resolved Hide resolved
homeassistant/components/switch/edp_redy.py Outdated Show resolved Hide resolved
homeassistant/components/switch/edp_redy.py Outdated Show resolved Hide resolved
homeassistant/components/switch/edp_redy.py Outdated Show resolved Hide resolved
homeassistant/components/switch/edp_redy.py Outdated Show resolved Hide resolved
homeassistant/components/switch/edp_redy.py Outdated Show resolved Hide resolved
@abmantis
Copy link
Contributor Author

abmantis commented Sep 9, 2018

Thanks for the suggestions @MartinHjelmare ! Nice catch on the entity updates refactoring, it is better now.

@home-assistant home-assistant deleted a comment from houndci-bot Sep 9, 2018
houndci-bot
houndci-bot reviewed Sep 9, 2018
View reviewed changes
homeassistant/components/edp_redy.py Outdated
await discovery.async_load_platform(hass, component,
DOMAIN, {}, config)
platform_loaded = True

Copy link

Choose a reason for hiding this comment

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

blank line contains whitespace

@abmantis
Copy link
Contributor Author

Is there any way to fix the following lint issue? Or is it a false positive?
homeassistant/components/edp_redy.py:57:19: E0601: Using variable 'platform_loaded' before assignment (used-before-assignment)

@MartinHjelmare
Copy link
Member

Not sure. Logic looks sane to me.

@abmantis
Copy link
Contributor Author

OK, thanks. Do you see anything else to do before merging?

@MartinHjelmare
Copy link
Member

Disable that pylint error for now. After that I think we're good. 👍

@abmantis
Copy link
Contributor Author

@MartinHjelmare this still shows that you are requesting changes. Is there anything else or you just forgot to resolve?

@MartinHjelmare MartinHjelmare merged commit 9c9df79 into home-assistant:dev Sep 15, 2018
@ghost ghost removed the in progress label Sep 15, 2018
@abmantis abmantis deleted the platform_edp_redy branch September 15, 2018 23:18
@balloob balloob mentioned this pull request Sep 28, 2018
Merged
@home-assistant home-assistant locked and limited conversation to collaborators Feb 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@MartinHjelmare MartinHjelmare MartinHjelmare approved these changes

Assignees
No one assigned
Labels
cla-signed integration: edp_redy new-platform
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@abmantis @MartinHjelmare @houndci-bot @homeassistant