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

New fan platform airscape #26889

Closed
wants to merge 8 commits into from
Closed

New fan platform airscape #26889

wants to merge 8 commits into from

Conversation

quielb
Copy link
Contributor

@quielb quielb commented Sep 24, 2019

Description:

Adding Airscape platform for the fan component to control Airscape Whole House Fans

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

Example entry for configuration.yaml (if applicable):

fan:
  - platform: airscape
    name: "Whole House Fan"
    host: "192.168.10.100"

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

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 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.

If the code does not interact with devices:

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

@quielb
Copy link
Contributor Author

quielb commented Sep 24, 2019

black formatting conflicts with pycodestyle line length checks:

Black wants to change the following (only one example, there are several)

-        except (
-            airscape.exceptions.ConnectionError,
-            airscape.exceptions.Timeout
-        ):
+        except (airscape.exceptions.ConnectionError, airscape.exceptions.Timeout):

But if I let black do that, then pycodestyle complains about line length:

fan.py:15:80: E501 line too long (80 > 79 characters)
fan.py:45:80: E501 line too long (83 > 79 characters)
fan.py:118:80: E501 line too long (82 > 79 characters)
fan.py:125:80: E501 line too long (82 > 79 characters)
fan.py:132:80: E501 line too long (82 > 79 characters)
fan.py:140:80: E501 line too long (82 > 79 characters)
fan.py:159:80: E501 line too long (82 > 79 characters)
fan.py:169:80: E501 line too long (82 > 79 characters)

which one wins?

@frenck
Copy link
Member

frenck commented Sep 24, 2019

We only consider and use Black for code formatting and use the 88 char line length that Black uses by default.

@quielb
Copy link
Contributor Author

quielb commented Sep 25, 2019

Not sure how I missed the pylint errors in my local tox run but I did. Though it might be a rebase issue so I rebased to dev latest branch ( was still on .99 level now at .100 ) and still get the pylint errors with tox -e pylint. Not sure what to do with them. since ,my changes aren't related to pylint errors. Not sure how CI tests are passing for any PR at this point.

Also not sure why code coverage is complaining either. I updated .coveragerc and can't do unit tests because communicates with external device. So there won't be any coverage for this diff.

.coveragerc Outdated Show resolved Hide resolved
homeassistant/components/airscape/fan.py Show resolved Hide resolved
homeassistant/components/airscape/fan.py Outdated Show resolved Hide resolved
homeassistant/components/airscape/fan.py Outdated Show resolved Hide resolved
homeassistant/components/airscape/fan.py Outdated Show resolved Hide resolved
self._speed = None
self._available = True
self._minimum_speed = minimum
self._speed_list = [f"{i}" for i in range(0, 11)]
Copy link
Member

Choose a reason for hiding this comment

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

Only speeds available in the base fan component are allowed in the speed list. Try to map as best possible between those and the device speed settings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Do you want to wait with continuing here until the architecture issue is resolved? If so please close this PR while waiting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was hoping to restart the conversation in the architecture repo. But it doesn't seem like anyone has the bandwidth to deal with this right now. I guess the best thing to do is close this and just move my platform over to a custom component until the architecture issue gets resolved.

homeassistant/components/airscape/fan.py Outdated Show resolved Hide resolved
homeassistant/components/airscape/fan.py Show resolved Hide resolved
homeassistant/components/airscape/manifest.json Outdated Show resolved Hide resolved
homeassistant/components/fan/services.yaml Outdated Show resolved Hide resolved
@MartinHjelmare
Copy link
Member

Try rebasing on latest dev branch to let the build pass without non related errors.

@quielb
Copy link
Contributor Author

quielb commented Sep 28, 2019

Try rebasing on latest dev branch to let the build pass without non related errors.

I rebased to latest dev branch and I am still get non related errors.

@quielb
Copy link
Contributor Author

quielb commented Sep 28, 2019

Try rebasing on latest dev branch to let the build pass without non related errors.

I rebased to latest dev branch and I am still get non related errors.

Looking deeper into my tox -e pylint errors I'm seeing import problems. There is something wrong with my venv. I tried rebuilding it, but I'm still having issues. I'm going to push the changes that I have and see what happens with the CI test pipeline while I try to resolve the other issues in my venv

@quielb quielb closed this Sep 30, 2019
@lock lock bot locked and limited conversation to collaborators Oct 1, 2019
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.

4 participants