-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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 fritzbox_callmonitor #40736
Add config flow to fritzbox_callmonitor #40736
Conversation
…-config-flow-fritz-box-call-monitor
…-config-flow-fritz-box-call-monitor
I will add tests for this integration as soon as I am sure that this config flow satisfies all standards. Therefore any feedback or comments about the config flow and the integrations itself is greatly appreciated 😄 |
I think if you can avoid a breaking change initially it's a good thing. You can do import from platforms to config entry just as well from the init https://github.com/home-assistant/core/pull/26587/files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of changes necessary 👍 Already looking really good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requires tests of the config flow to be added.
Removing the breaking change paragraph and label since this breaking change has been merged with #39995 |
…-config-flow-fritz-box-call-monitor
…-config-flow-fritz-box-call-monitor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Some comments.
try: | ||
return self.number_dict[prefix + number.lstrip("0")] | ||
except KeyError: | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should return None explicitly in the last case.
"homeassistant.components.fritzbox_callmonitor.base.FritzPhonebook.modelname", | ||
return_value=MOCK_PHONEBOOK_NAME_1, | ||
), patch( | ||
"homeassistant.components.fritzbox_callmonitor.config_flow.FritzConnection.__init__", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally the class name is patched and not the init method. Then the return value of that mock, of the class, can be specced with attributes that have return values or side effects as needed.
port=self._port, | ||
sensor=self, | ||
) | ||
self._monitor.connect() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The connect does blocking I/O by creating and connecting a socket. This needs to run in the executor via hass.async_add_executor_job
.
Breaking change
Proposed change
Adds the ability to set up the
fritzbox_callmonitor
integration via a config flow and adds the ability to import from YAML to a config entry.Type of change
Example entry for
configuration.yaml
:Additional information
Checklist
black --fast homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: