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.

Sign up for GitHub

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

Jump to bottom

Add config flow to Netgear LTE #92632

Closed
wants to merge 5 commits into from
Closed

Add config flow to Netgear LTE #92632

wants to merge 5 commits into from

Conversation

tkdrob
Copy link
Contributor

@tkdrob tkdrob commented May 5, 2023 •
edited

Breaking change

Netgear LTE can now be set up in the UI. Existing yaml configurations are automatically imported and can be safely removed. The Netgear LTE notify service was previously set up via YAML configuration. This created a service for a specified recipient without having to include the phone number. These services are still available for now. Please adjust any automations or scripts you may have to use the new service and include target for specifying a recipient.

Proposed change

Add config flow to Netgear LTE.
There are a lot of other changes that should be done but have been left out to keep the runtime changes in this PR smaller. I plan on doing followups for the rest.

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
  • I have followed the perfect PR recommendations
  • 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:

@tkdrob tkdrob mentioned this pull request May 5, 2023
Merged
8 tasks
@tkdrob tkdrob marked this pull request as ready for review May 5, 2023 16:33
epenet
epenet requested changes May 10, 2023
View reviewed changes
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.

Please split this PR.
It contains a lot of refactoring that can be done in a preliminary or follow-up PR.

homeassistant/components/netgear_lte/manifest.json Show resolved Hide resolved
homeassistant/components/netgear_lte/notify.py Outdated
Comment on lines 13 to 14
from . import CONF_NOTIFY, CONF_RECIPIENT
from .const import DOMAIN
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest that you create const.py in a preliminary PR, with DOMAIN, CONF_NOTIFY and CONF_RECIPIENT.

homeassistant/components/netgear_lte/binary_sensor.py Outdated

from . import CONF_BINARY_SENSOR, CONF_MONITORED_CONDITIONS, DATA_KEY, LTEEntity
from . import LTEEntity
Copy link
Contributor

Choose a reason for hiding this comment

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

In a preliminary PR you could move this to entity.py

homeassistant/components/netgear_lte/sensor.py Outdated
from homeassistant.components.sensor import (
SensorDeviceClass,
SensorEntity,
SensorEntityDescription,
Copy link
Contributor

Choose a reason for hiding this comment

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

SensorEntityDescription could be done in a separate preliminary PR

homeassistant/components/netgear_lte/binary_sensor.py Outdated
from homeassistant.components.binary_sensor import BinarySensorEntity
from homeassistant.components.binary_sensor import (
BinarySensorEntity,
BinarySensorEntityDescription,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think BinarySensorEntityDescription could be implemented in a separate PR (preliminary or follow-up)

homeassistant/components/netgear_lte/__init__.py
SERVICE_CONNECT_LTE: CONNECT_LTE_SCHEMA,
SERVICE_DISCONNECT_LTE: DISCONNECT_LTE_SCHEMA,
}
hass.data[DATA_HASS_CONFIG] = config
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should move this under the DOMAIN key, either as a dict with sub-items or possibly as a dataclass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This constants resolves to "netgear_lte_hass_config". Much will eventually be moved to coordinator and leave just the notify service using it.

@home-assistant home-assistant bot marked this pull request as draft May 10, 2023 08:50
@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.

This was referenced May 11, 2023
Merged
Merged
Merged
@tkdrob
Copy link
Contributor Author

tkdrob commented May 12, 2023

Botched commit. Closing to resubmit.

@tkdrob tkdrob closed this May 12, 2023
@tkdrob tkdrob deleted the netgear_lte_config_flow branch May 12, 2023 17:21
@github-actions github-actions bot locked and limited conversation to collaborators May 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@epenet epenet epenet requested changes

@Quentame Quentame Awaiting requested review from Quentame Quentame will be requested when the pull request is marked ready for review Quentame is a code owner

@mib1185 mib1185 Awaiting requested review from mib1185 mib1185 will be requested when the pull request is marked ready for review mib1185 is a code owner

@Swamp-Ig Swamp-Ig Awaiting requested review from Swamp-Ig Swamp-Ig will be requested when the pull request is marked ready for review Swamp-Ig is a code owner

@gjohansson-ST gjohansson-ST Awaiting requested review from gjohansson-ST gjohansson-ST will be requested when the pull request is marked ready for review gjohansson-ST is a code owner

@starkillerOG starkillerOG Awaiting requested review from starkillerOG starkillerOG will be requested when the pull request is marked ready for review starkillerOG is a code owner

@frenck frenck Awaiting requested review from frenck frenck will be requested when the pull request is marked ready for review frenck is a code owner

@joostlek joostlek Awaiting requested review from joostlek joostlek will be requested when the pull request is marked ready for review joostlek is a code owner

@hunterjm hunterjm Awaiting requested review from hunterjm hunterjm will be requested when the pull request is marked ready for review hunterjm is a code owner

@emontnemery emontnemery Awaiting requested review from emontnemery emontnemery will be requested when the pull request is marked ready for review emontnemery is a code owner

@jbouwh jbouwh Awaiting requested review from jbouwh jbouwh will be requested when the pull request is marked ready for review jbouwh is a code owner

@bdraco bdraco Awaiting requested review from bdraco bdraco will be requested when the pull request is marked ready for review bdraco is a code owner

@chemelli74 chemelli74 Awaiting requested review from chemelli74 chemelli74 will be requested when the pull request is marked ready for review chemelli74 is a code owner

@Danielhiversen Danielhiversen Awaiting requested review from Danielhiversen Danielhiversen will be requested when the pull request is marked ready for review Danielhiversen is a code owner

Assignees
No one assigned
Labels
cla-signed config-flow This integration migrates to the UI by adding a config flow has-tests integration: netgear_lte new-feature Quality Scale: No score
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@tkdrob @epenet