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 camera platform to Freebox #88104

Merged
merged 85 commits into from
Apr 25, 2023
Merged

Conversation

nachonam
Copy link
Contributor

@nachonam nachonam commented Feb 14, 2023

Breaking change

Proposed change

Plan for implementing all Freebox Home sensors (cameras, pir,dws,remote commands..) and alarm system.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

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.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

@home-assistant
Copy link

Hey there @hacf-fr, @Quentame, mind taking a look at this pull request as it has been labeled with an integration (freebox) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of freebox can trigger bot actions by commenting:

  • @home-assistant close Closes the issue.
  • @home-assistant rename Awesome new title Change the title of the issue.
  • @home-assistant reopen Reopen the issue.
  • @home-assistant unassign freebox Removes the current integration label and assignees on the issue, add the integration domain after the command.

@nachonam
Copy link
Contributor Author

Hello,
A little PR to add Freebox cameras to HA
Regards

@BenoitAnastay
Copy link

Also you do not seem to have updated units tests

https://github.com/home-assistant/core/tree/dev/tests/components/freebox

@nachonam
Copy link
Contributor Author

Also you do not seem to have updated units tests

https://github.com/home-assistant/core/tree/dev/tests/components/freebox

I'm working on... 75% covered

@emontnemery
Copy link
Contributor

I'm marking this as draft, please set to ready for review when the tests have been updated 👍

@emontnemery emontnemery marked this pull request as draft February 20, 2023 13:41
@nachonam
Copy link
Contributor Author

I'm marking this as draft, please set to ready for review when the tests have been updated +1

88% is enough?

> --------- coverage: platform linux, python 3.10.9-final-0 -----------
> Name                                              Stmts   Miss  Cover   Missing
> -------------------------------------------------------------------------------
> homeassistant/components/freebox/__init__.py         50      2    96%   54-55
> homeassistant/components/freebox/base_class.py       99     43    57%   38-39, 49-54, 69, 89-97, 101-109, 113-125, 134-137, 142-154, 158-171
> homeassistant/components/freebox/button.py           32      0   100%
> homeassistant/components/freebox/camera.py          113     11    90%   72, 144, 149, 153, 157-158, 170-173, 178-181, 191
> homeassistant/components/freebox/config_flow.py      57      0   100%
> homeassistant/components/freebox/const.py            28      0   100%
> homeassistant/components/freebox/router.py          128      7    95%   43, 163-165, 182, 234, 244
> -------------------------------------------------------------------------------
> TOTAL                                               507     63    88%

@BenoitAnastay
Copy link

88% is enough?

I was looking for answers in the documentation but I do not find any specified value except the use of the word full.

But I guess it's enough, since there is tests and they do work it's fine.

@nachonam
Copy link
Contributor Author

88% is enough?

I was looking for answers in the documentation but I do not find any specified value except the use of the word full.

But I guess it's enough, since there is tests and they do work it's fine.

Ok, good...
So can I commit the last modifications to this PR or I need to close this PR and open another one?

@BenoitAnastay
Copy link

Copy link
Member

@Quentame Quentame left a comment

Choose a reason for hiding this comment

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

Some changes to be made.

Thanks to start implementing the Home API of the Freebox.

Fill the "Proposed change" section of the PR and your plan for implementing all Freebox Home sensors.

homeassistant/components/freebox/const.py Outdated Show resolved Hide resolved
homeassistant/components/freebox/base_class.py Outdated Show resolved Hide resolved
homeassistant/components/freebox/base_class.py Outdated Show resolved Hide resolved
homeassistant/components/freebox/base_class.py Outdated Show resolved Hide resolved
homeassistant/components/freebox/base_class.py Outdated Show resolved Hide resolved
homeassistant/components/freebox/router.py Outdated Show resolved Hide resolved
homeassistant/components/freebox/router.py Outdated Show resolved Hide resolved
homeassistant/components/freebox/camera.py Outdated Show resolved Hide resolved
homeassistant/components/freebox/camera.py Outdated Show resolved Hide resolved
homeassistant/components/freebox/base_class.py Outdated Show resolved Hide resolved
Copy link
Member

@Quentame Quentame left a comment

Choose a reason for hiding this comment

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

Please take a look at your documentation PR review: home-assistant/home-assistant.io#26669

nachonam added a commit to nachonam/home-assistant.io that referenced this pull request Apr 18, 2023
.coveragerc Show resolved Hide resolved
Quentame pushed a commit to nachonam/home-assistant.io that referenced this pull request Apr 19, 2023
.coveragerc Show resolved Hide resolved
.coveragerc Outdated Show resolved Hide resolved
Copy link
Member

@Quentame Quentame left a comment

Choose a reason for hiding this comment

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

LGTM

@Quentame Quentame changed the title Add Freebox cameras Add camera platform to Freebox Apr 25, 2023
@Quentame Quentame merged commit 2d510bf into home-assistant:dev Apr 25, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Apr 26, 2023
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