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 config flow to Roth Touchline #83484

Closed

Conversation

pilehave
Copy link
Contributor

@pilehave pilehave commented Dec 7, 2022

Breaking change

Proposed change

  • Add setup from config flow
  • Allow some flexibility on user input of the address of the control unit (with/without http)
  • Add unique id to each thermostat to allow for area assignement etc.

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:

@pilehave pilehave marked this pull request as draft December 7, 2022 17:33
@pilehave pilehave marked this pull request as ready for review December 7, 2022 17:33
@pilehave pilehave mentioned this pull request Dec 7, 2022
19 tasks
Copy link
Contributor

@bachya bachya left a comment

Choose a reason for hiding this comment

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

Additionally, I'm not an expert on how the translations system works, but I don't think this PR should be altering various non-English translations manually.

homeassistant/components/touchline/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/touchline/climate.py Outdated Show resolved Hide resolved
homeassistant/components/touchline/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/touchline/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/touchline/config_flow.py Outdated Show resolved Hide resolved
@pilehave
Copy link
Contributor Author

pilehave commented Dec 8, 2022

Additionally, I'm not an expert on how the translations system works, but I don't think this PR should be altering various non-English translations manually.

The "translations"-folder and files in it? Seems to be the way other integratrions are doing it. When I browse the code, most of them has the same folder and a JSON-file for each language. I just copied from a similar integration and removed a unused string from each of them.

@bachya
Copy link
Contributor

bachya commented Dec 8, 2022

Integrations don't add those files manually; they are generated by Lokalise automatically. When an integration adds strings.yaml, the PR should only include the English translations file (generated with python3 -m script.translations develop).

@pilehave
Copy link
Contributor Author

I have removed the manually added translations. I don't get why there is a "docs-missing — Please open a documentation PR." failed check. It should be ready to merge?

@pilehave pilehave requested a review from bachya January 3, 2023 12:56
homeassistant/components/touchline/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/touchline/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/touchline/climate.py Show resolved Hide resolved
homeassistant/components/touchline/climate.py Outdated Show resolved Hide resolved
homeassistant/components/touchline/climate.py Outdated Show resolved Hide resolved
homeassistant/components/touchline/climate.py Outdated Show resolved Hide resolved
homeassistant/components/touchline/climate.py Outdated Show resolved Hide resolved
homeassistant/components/touchline/climate.py Outdated Show resolved Hide resolved
homeassistant/components/touchline/climate.py Outdated Show resolved Hide resolved
@pilehave
Copy link
Contributor Author

Documentation has been updated - should be ready to merge this PR as well.

@pilehave pilehave requested a review from bachya January 24, 2023 21:02
@pfwilliam
Copy link

Hi @pilehave , any idea on when this update might get released? :) I would love the unique id to be added, so I can add the floor heating to my automations in node red.
thank you for awesome work!

@KimCarlsen
Copy link

@bachya any comments on this PR? thanks

Copy link
Contributor

@bachya bachya left a comment

Choose a reason for hiding this comment

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

Approved once CI is addressed.

Copy link
Contributor

@epenet epenet left a comment

Choose a reason for hiding this comment

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

config-flow requires 100% coverage, yet tests are missing.
Please address the missing tests

@@ -0,0 +1,20 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is no longer necessary. Please remove.

@home-assistant home-assistant bot marked this pull request as draft March 22, 2023 15:51
@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@pilehave
Copy link
Contributor Author

I don't know how to add tests. Please advice and point to relevant guide in documentation.

@KimCarlsen
Copy link

@pilehave did you look at the documentation for tests? https://developers.home-assistant.io/docs/development_testing#writing-tests-for-integrations

@dehoej
Copy link

dehoej commented Jul 26, 2023

Is there anything we can do to get this merged?

@frenck
Copy link
Member

frenck commented Oct 6, 2023

Because there hasn't been any activity on this PR for quite some time now, I've decided to close it for being stale.

Feel free to re-open this PR when you are ready to pick up work on it again 👍

../Frenck

@frenck frenck closed this Oct 6, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Oct 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing unique ID on touchline integration
7 participants