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 TaHoma integration (base + Binary Sensor) #43966

Conversation

iMicknl
Copy link
Contributor

@iMicknl iMicknl commented Dec 5, 2020

Proposed change

Since #41267 is not easy to review, we (@vlebourl, @tetienne and me) will split the work done in https://github.com/imicknl/ha-tahoma in multiple small PR's. This PR includes some of the ground work for other platforms, but all non necessary code has been removed to keep it as small as possible.

  • Add new Somfy TaHoma domain and integration, using Config Flow and DataUpdateCoordinator.
  • Support for Binary Sensors, other platforms will be added in other PR's.

Would be great to PR this to a feature branch, instead of dev, since I would like to have all platforms (especially cover) included in a core release.

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)
  • 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.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

  • I have reviewed two other [open pull requests][prs] in this repository.
    • 37717
    • 40626

@frenck
Copy link
Member

frenck commented Dec 5, 2020

Why the new domain? If the old integration is broken, you can just replace it, right?

@frenck frenck mentioned this pull request Dec 5, 2020
21 tasks
@iMicknl
Copy link
Contributor Author

iMicknl commented Dec 5, 2020

Why the new domain? If the old integration is broken, you can just replace it, right?

See discussion at #43952 (comment).

@iMicknl iMicknl changed the base branch from dev to rework-tahoma December 5, 2020 21:43
@iMicknl
Copy link
Contributor Author

iMicknl commented Dec 6, 2020

Why the new domain? If the old integration is broken, you can just replace it, right?

I am not sure what we all agreed on in the end in #43952. Should we change this PR to target tahoma instead of somfy_tahoma, or shall we take this opportunity to update the domain to a more suitable name?

Our reason for targeting somfy_tahoma is mainly because this integration is called Somfy TaHoma nowadays and it is closely related to the original somfy integration. It would be good to have them close in the code, like somfy_mylink, to ease the discovery for new developers. I don't consider it a breaking change since the current integration is not in use and broken for many months, but if the core team doesn't allow this change, we will change it back to tahoma.

Target branch change to 'rework-tahoma'.

@frenck
Copy link
Member

frenck commented Dec 6, 2020

I would say; keep the domain as is. Another helpful thing for users; is migration. The username/password from an existing tahoma config can be imported as a config entry.

@iMicknl iMicknl force-pushed the feature/add_somfy_tahoma_integration branch from 08b465c to 03992d6 Compare January 8, 2021 14:47
@iMicknl
Copy link
Contributor Author

iMicknl commented Jan 8, 2021

I would say; keep the domain as is. Another helpful thing for users; is migration. The username/password from an existing tahoma config can be imported as a config entry.

Done! This PR will now target the tahoma domain.

I will add back the YAML config support in a separate PR to keep this PR small, since we now target a feature branch.

@iMicknl iMicknl changed the title Add Somfy TaHoma integration Add TaHoma integration (base + Binary Sensor) Jan 8, 2021
@iMicknl iMicknl force-pushed the feature/add_somfy_tahoma_integration branch 3 times, most recently from 698479e to d93a410 Compare January 14, 2021 18:17
@iMicknl
Copy link
Contributor Author

iMicknl commented Jan 14, 2021

Lately, I accidentally rebased on latest dev, but I forgot that the target is rework-tahoma. Force pushed a fixed version rebased on rework-tahoma.

If this PR is reviewed / merged to the work branch, I will add all other (small) PR's, however we first need to have the base merged for that.

@probot-home-assistant
Copy link

Hey there @philklei, mind taking a look at this pull request as its been labeled with an integration (tahoma) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

@github-actions
Copy link

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@github-actions github-actions bot added the stale label Feb 14, 2021
homeassistant/components/tahoma/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/tahoma/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/tahoma/binary_sensor.py Outdated Show resolved Hide resolved
homeassistant/components/tahoma/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/tahoma/coordinator.py Outdated Show resolved Hide resolved
homeassistant/components/tahoma/tahoma_device.py Outdated Show resolved Hide resolved
homeassistant/components/tahoma/tahoma_device.py Outdated Show resolved Hide resolved
homeassistant/components/tahoma/tahoma_device.py Outdated Show resolved Hide resolved
homeassistant/components/tahoma/tahoma_device.py Outdated Show resolved Hide resolved
homeassistant/components/tahoma/tahoma_device.py Outdated Show resolved Hide resolved
Dev automation moved this from Needs review to Review in progress Feb 14, 2021
@iMicknl iMicknl force-pushed the feature/add_somfy_tahoma_integration branch from 21c1b1e to c4c28a9 Compare February 15, 2021 20:16
@iMicknl
Copy link
Contributor Author

iMicknl commented Feb 21, 2021

Processed all feedback and all checks have passed. ✅

.coveragerc Show resolved Hide resolved
homeassistant/components/tahoma/binary_sensor.py Outdated Show resolved Hide resolved
homeassistant/components/tahoma/binary_sensor.py Outdated Show resolved Hide resolved
homeassistant/components/tahoma/tahoma_entity.py Outdated Show resolved Hide resolved
@iMicknl
Copy link
Contributor Author

iMicknl commented Feb 22, 2021

Feedback of second round processed, hopefully all checks will pass again.

Minor question, after we removed translations/en.json for this PR, the dialog is not showing any strings. Is this a mistake on our side, or will this be fixed after the first sync with Lokalise?

Screen Shot 2021-02-22 at 13 50 53

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.

Looks good!

Dev automation moved this from Review in progress to Reviewer approved Feb 22, 2021
@MartinHjelmare
Copy link
Member

Yeah, translations are needed to test the frontend, and this will be updated by our infrastructure and Lokalise. But we do allow committing the default English translation file for testing purposes and convenience.

@iMicknl
Copy link
Contributor Author

iMicknl commented Feb 22, 2021

Yeah, translations are needed to test the frontend, and this will be updated by our infrastructure and Lokalise. But we do allow committing the default English translation file for testing purposes and convenience.

Ok, I will add the English one to this PR. Thanks!

Copy link
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

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

LGTM

"manufacturer": manufacturer,
"name": self.name,
"model": model,
"sw_version": self.device.controllable_name,
Copy link
Member

Choose a reason for hiding this comment

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

I think this device can set suggested_area here in a future PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We created it as a work item already on our custom component repo, iMicknl/ha-tahoma#392 :). However, it would require pulling the TaHoma area's first and that endpoint hasn't been implemented yet in our client.

@MartinHjelmare
Copy link
Member

Please link a docs PR in the PR description. Make a note in the docs PR that there will be multiple parent PRs in the core repo before the docs PR should be merged. Then we can merge here.

@iMicknl
Copy link
Contributor Author

iMicknl commented Feb 22, 2021

Please link a docs PR in the PR description. Make a note in the docs PR that there will be multiple parent PRs in the core repo before the docs PR should be merged. Then we can merge here.

@MartinHjelmare is it ok to add the docs for the final PR to dev? This PR is currently targeting a work branch. Otherwise, I would need to do a docs PR per PR, which is quite some work. I expect 10+ follow up PR's, since we will split this up by platform and amount of code change.

update:
Added the original documentation update, for the previous core PR. Will work on making sure this is fully up to date with rework-tahoma branch.

@MartinHjelmare
Copy link
Member

Yeah, the idea is that we use a single docs PR for this work.

@MartinHjelmare MartinHjelmare merged commit 8d6f642 into home-assistant:rework-tahoma Feb 22, 2021
Dev automation moved this from Reviewer approved to Done Feb 22, 2021
@iMicknl iMicknl deleted the feature/add_somfy_tahoma_integration branch February 22, 2021 16:17
@github-actions github-actions bot locked and limited conversation to collaborators Feb 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
Development

Successfully merging this pull request may close these issues.

Tahoma ( Somfy ) update 30.6.2020 Tahoma component stopped working after Connexoon firmware last update
7 participants