Skip to content

Add Unifi Led#27475

Merged
MartinHjelmare merged 12 commits into
home-assistant:devfrom
florisvdk:unifiled
Oct 24, 2019
Merged

Add Unifi Led#27475
MartinHjelmare merged 12 commits into
home-assistant:devfrom
florisvdk:unifiled

Conversation

@florisvdk
Copy link
Copy Markdown
Contributor

@florisvdk florisvdk commented Oct 11, 2019

Description:
Added Ubiquiti Unifi LED basic light platform (On/Off and brightness)

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

Example entry for configuration.yaml (if applicable):

light:
  - platform: unifiled
    host: 192.168.1.20
    port: 20443
    username: ubnt
    password: ubnt

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.

@homeassistant

This comment has been minimized.

@homeassistant

This comment has been minimized.

Copy link
Copy Markdown
Member

@dshokouhi dshokouhi left a comment

Choose a reason for hiding this comment

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

Just a couple of comments based on my experience from PR's I submitted

Comment thread homeassistant/components/unifiled/light.py Outdated
Comment thread homeassistant/components/unifiled/light.py Outdated
Comment thread homeassistant/components/unifiled/light.py Outdated
Comment thread homeassistant/components/unifiled/light.py Outdated
@MartinHjelmare MartinHjelmare changed the title Added Unifi Led Add Unifi Led Oct 12, 2019
Copy link
Copy Markdown
Contributor Author

@florisvdk florisvdk left a comment

Choose a reason for hiding this comment

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

Everything noted should be fixed.

@florisvdk florisvdk mentioned this pull request Oct 14, 2019
9 tasks
@florisvdk
Copy link
Copy Markdown
Contributor Author

@dshokouhi If you can have a look at the changes i did it would be greatly appreciated.

Copy link
Copy Markdown
Member

@dshokouhi dshokouhi 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, will need an approval from someone who has merge rights to move forward.

Copy link
Copy Markdown
Member

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

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

Looks generally good to me, I added a couple of things I noticed that I have been notified before.

edit: btw, you seem to link on an unrelated PR in the description (first line)

Comment thread homeassistant/components/unifiled/light.py
Comment thread homeassistant/components/unifiled/light.py Outdated
Comment thread homeassistant/components/unifiled/light.py
Copy link
Copy Markdown
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! Some small changes needed.

Comment thread homeassistant/components/unifiled/light.py Outdated
Comment thread homeassistant/components/unifiled/light.py Outdated
Comment thread homeassistant/components/unifiled/manifest.json Outdated
@MartinHjelmare
Copy link
Copy Markdown
Member

Please run black code formatter from the project root.

black --fast homeassistant

@florisvdk
Copy link
Copy Markdown
Contributor Author

florisvdk commented Oct 24, 2019

This is what i get:

(venv) floris@Ha-dev-vm:~/home-assistant$ black --fast homeassistant
All done! ✨ 🍰 ✨
2632 files left unchanged.

And i get the same without the fast option.

@MartinHjelmare
Copy link
Copy Markdown
Member

And there are no changes in your local branch that aren't pushed yet to the PR?

@florisvdk
Copy link
Copy Markdown
Contributor Author

No changes that are not committed and pushed.

@florisvdk
Copy link
Copy Markdown
Contributor Author

I am not finding the thing it is failing on, all the commands i try return that it is good.
I have tried different options of black, flake8 and script/lint

@florisvdk
Copy link
Copy Markdown
Contributor Author

The last commit is the same as a previous one that did not fail so something is broken.

@florisvdk
Copy link
Copy Markdown
Contributor Author

@MartinHjelmare Looks like this is the cause of the issue: #28171

@MartinHjelmare
Copy link
Copy Markdown
Member

The PR to fix the formatting is merged. Try rebasing on latest dev branch to let the build pass.

Copy link
Copy Markdown
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.

Thanks!

@MartinHjelmare MartinHjelmare merged commit b1fcecd into home-assistant:dev Oct 24, 2019
@lock lock Bot locked and limited conversation to collaborators Oct 25, 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.

6 participants