Add coordinator to 17Track #115057
Add coordinator to 17Track #115057
Conversation
|
There are more in here than just adding a DUC. So I would suggest you break out the refactoring and entity descriptions into their own PR's to make it more feasible to review. Thanks |
remove SensorEntityDescription (different PR)
|
@gjohansson-ST I removed the SensorEntityDescription |
|
|
||
| class SeventeenTrackData: | ||
| """Class for handling the data retrieval.""" | ||
|
|
||
| def __init__(self) -> None: | ||
| """Initialize the data object.""" | ||
| self.summary: dict[str, dict[str, Any]] = {} | ||
| self.current_packages: dict[str, dict[str, Any]] = {} | ||
| self.new_packages: dict[str, Package] = {} | ||
| self.old_packages: set[Package] = set() |
| current_packages = set( | ||
| await self._client.profile.packages(show_archived=self._show_archived) | ||
| ) |
| for status, quantity in summary.items(): | ||
| data.summary[slugify(status)] = { | ||
| "quantity": quantity, | ||
| "packages": [], | ||
| } | ||
|
|
There was a problem hiding this comment.
Keep things that can't raise out of the try block
| self._status = status | ||
| self._attr_name = f"Seventeentrack Packages {status}" | ||
| self._attr_unique_id = f"summary_{data.account_id}_{slugify(status)}" | ||
| self._attr_extra_state_attributes = {} |
| package_data = self.coordinator.data.current_packages.get( | ||
| self._tracking_number, {} | ||
| ) | ||
| package = package_data.get("package") | ||
| if package is None or not (name := package.friendly_name): |
There was a problem hiding this comment.
Afaik (I know for sure for native value and some other properties, name I am not sure), if the available property returns False, the other properties aren't requested anymore, so we shouldn't have to check again here.
(We can use square brackets instead of get
self.coordinator.data.current_packages[self._tracking_number]
)
There was a problem hiding this comment.
moved it anyway to init
| package_data = self.coordinator.data.current_packages.get( | ||
| self._tracking_number, {} | ||
| ) |
|
|
||
|
|
||
| class SeventeenTrackData: | ||
| """Define a data handler for 17track.net.""" | ||
| def remove_entity(hass: HomeAssistant, account_id: str, tracking_number: str) -> bool: |
There was a problem hiding this comment.
Can we maybe just pass the list of packages it should remove so it can do a for loop here?
| ) | ||
| def notify_delivered(hass: HomeAssistant, friendly_name: str, tracking_number: str): | ||
| """Notify when package is delivered.""" | ||
| LOGGER.info("Package delivered: %s", tracking_number) |
There was a problem hiding this comment.
Please don't use the info level of logging
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com>
Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com>
| def __init__(self) -> None: | ||
| """Initialize the data object.""" | ||
| self.summary: dict[str, dict[str, Any]] = {} | ||
| self.current_packages: dict[str, dict[str, Any]] = {} | ||
| self.new_packages: dict[str, Package] = {} | ||
| self.old_packages: set[Package] = set() |
There was a problem hiding this comment.
| def __init__(self) -> None: | |
| """Initialize the data object.""" | |
| self.summary: dict[str, dict[str, Any]] = {} | |
| self.current_packages: dict[str, dict[str, Any]] = {} | |
| self.new_packages: dict[str, Package] = {} | |
| self.old_packages: set[Package] = set() | |
| summary: dict[str, dict[str, Any]] | |
| current_packages: dict[str, dict[str, Any]] |
I think it's wise to actually just keep the list of package tracking number of the last update in a set in async_setup_entry in sensor.py. This would make the coordinator less responsible for creating and removing entities. (and it concentrates the logic that we can remove in a few months)
| data.current_packages[package.tracking_number] = { | ||
| "package": package, | ||
| "extra": { | ||
| ATTR_DESTINATION_COUNTRY: package.destination_country, | ||
| ATTR_INFO_TEXT: package.info_text, | ||
| ATTR_TIMESTAMP: package.timestamp, | ||
| ATTR_LOCATION: package.location, | ||
| ATTR_ORIGIN_COUNTRY: package.origin_country, | ||
| ATTR_PACKAGE_TYPE: package.package_type, | ||
| ATTR_TRACKING_INFO_LANGUAGE: package.tracking_info_language, | ||
| ATTR_TRACKING_NUMBER: package.tracking_number, | ||
| }, | ||
| } |
There was a problem hiding this comment.
If I recall correctly, these were the extra state attributes. I would keep that logic at the sensor.
| @property | ||
| def show_delivered(self): | ||
| """Return whether delivered packages should be shown.""" | ||
| return self._show_delivered | ||
|
|
||
| @property | ||
| def account_id(self): | ||
| """Return the account ID.""" | ||
| return self._account_id |
There was a problem hiding this comment.
instead of returning the self._something just assign the variable to self.something in the first place so you dont need to create separate properties
| SeventeenTrackPackageSensor( | ||
| coordinator, t_number, p_data.friendly_name, p_data.status | ||
| ) | ||
| for t_number, p_data in coordinator.data.new_packages.items() |
There was a problem hiding this comment.
I'd write out the variables, we write code for developers, not for the computer :)
| self._status = status | ||
| self._attr_name = f"Seventeentrack Packages {status}" | ||
| self._attr_unique_id = f"summary_{data.account_id}_{slugify(status)}" | ||
| self._attr_name = f"Seventeentrack Packages {self._status}" |
There was a problem hiding this comment.
| self._attr_name = f"Seventeentrack Packages {self._status}" | |
| self._attr_name = f"Seventeentrack Packages {status}" |
|
|
||
| @property | ||
| def available(self) -> bool: | ||
| """Return whether the entity is available.""" | ||
| return self._state is not None | ||
| return self.coordinator.data.summary[self._status]["quantity"] is not None |
There was a problem hiding this comment.
When this is unavalable, is summary[status] even set? Shouldn't it be
| return self.coordinator.data.summary[self._status]["quantity"] is not None | |
| return self._status in self.coordinator.data.summary |
| name = friendly_name if friendly_name else tracking_number | ||
| self._name = f"Seventeentrack Package: {name}" |
There was a problem hiding this comment.
there is no need to calculate this each time property name is called.
We have the information at init time
There was a problem hiding this comment.
Let's leave these optimizations for a future PR, we can use shorthand attributes for this for example
fix second CR
remove commented out code + fix display name
| ) | ||
| self.show_delivered = self.config_entry.options[CONF_SHOW_DELIVERED] | ||
| self.account_id = client.profile.account_id | ||
| self.data = SeventeenTrackData() |
There was a problem hiding this comment.
Why don't you construct a new one every time?
|
|
||
| @callback | ||
| def _async_create_remove_entities(): | ||
| old_packages = coordinator.data.current_packages - set( | ||
| coordinator.data.live_packages.values() | ||
| ) | ||
| packages_to_add = ( | ||
| set(coordinator.data.live_packages.values()) | ||
| - coordinator.data.current_packages | ||
| ) |
There was a problem hiding this comment.
You can create a set outside of this function with all the tracking numbers for which you already made sensors, that would simplify the coordinator and this function
created a set outside _async_create_remove_entities function
…u-add-coordinator
| summary = {} | ||
| live_packages = set() |
There was a problem hiding this comment.
why? I need it when getting the packages from the API.
Then I populate a dict [str, Package] to return as the coordinator data
| except SeventeenTrackError as err: | ||
| LOGGER.error("There was an error retrieving the packages: %s", err) | ||
|
|
||
| data.live_packages.clear() |
| if summary_value: | ||
| summary_value["packages"].append(package) | ||
|
|
||
| return data |
There was a problem hiding this comment.
I would almost say like, just make it like you return
return SeventeenTrackData(packages=...)
There was a problem hiding this comment.
Not sure what you mean? the SeventeenTrackData is being constructed along the way
| @@ -111,242 +108,187 @@ async def async_setup_entry( | |||
| ) -> None: | |||
| """Set up a 17Track sensor entry.""" | |||
|
|
|||
| client = hass.data[DOMAIN][config_entry.entry_id] | |||
| coordinator: SeventeenTrackCoordinator = hass.data[DOMAIN][config_entry.entry_id] | |||
| current_packages: set[Package] = set() | |||
There was a problem hiding this comment.
Since the content of the set is unique, i am not sure if package works as type. If we can prove that this works, great, otherwise i think tracking numbers are the way to go
There was a problem hiding this comment.
Package holds the tracking number so it's unique. Also I need the Package itself for the various data it holds
There was a problem hiding this comment.
I know it's unique for us, but I'm talking about unique in the set. If a package is set to "Delivered", is that counted as a different object? If so, this code would not work
There was a problem hiding this comment.
right.
So I added a clear() to the set before I update() it. That solves it
There was a problem hiding this comment.
Checkout
core/homeassistant/components/opensky/coordinator.py
Lines 85 to 90 in 988c71e
This is what I use in Opensky to track the flights.
| try: | ||
| summary = await self._client.profile.summary( | ||
| show_archived=self._show_archived | ||
| ) | ||
|
|
||
| except SeventeenTrackError as err: | ||
| LOGGER.error("There was an error retrieving the summary: %s", err) | ||
|
|
||
| for status, quantity in summary.items(): | ||
| summary_dict[slugify(status)] = { | ||
| "quantity": quantity, | ||
| "packages": [], | ||
| "status_name": status, | ||
| } | ||
|
|
||
| try: | ||
| live_packages = set( | ||
| await self._client.profile.packages(show_archived=self._show_archived) | ||
| ) | ||
| except SeventeenTrackError as err: | ||
| LOGGER.error("There was an error retrieving the packages: %s", err) |
There was a problem hiding this comment.
Should we raise UpdateFailed when one of these 2 calls fail?
There was a problem hiding this comment.
I'm not sure what is the added value for raising it. If either has an error we can continue with empty data for one but the other will have data populated
There was a problem hiding this comment.
How stable is the API? If its not excessively flakey I would suggest just raising it
There was a problem hiding this comment.
These 2 calls can now be merged
Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com>
raise UpdateFailed if API throws an exception
merge calls
* Add coordinator to 17Track * Add coordinator to 17Track remove SensorEntityDescription (different PR) * Update homeassistant/components/seventeentrack/coordinator.py Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com> * Update homeassistant/components/seventeentrack/sensor.py Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com> * Add coordinator to 17Track fix CR * Add coordinator to 17Track fix second CR * Add coordinator to 17Track remove commented out code + fix display name * Add coordinator to 17Track created a set outside _async_create_remove_entities function * Add coordinator to 17Track fix CR * Add coordinator to 17Track fix CR 2 * Update homeassistant/components/seventeentrack/coordinator.py Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com> * Add coordinator to 17Track raise UpdateFailed if API throws an exception * Add coordinator to 17Track merge calls --------- Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com>
Proposed change
Add coordinator to 17Track
Type of change
Additional information
Checklist
ruff format 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.To help with the load of incoming pull requests: