-
Notifications
You must be signed in to change notification settings - Fork 33
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
Fix for getting the gateway. #253
Conversation
This seems like a better path forward than #252. Thanks for the work! |
@wbyoung Can you test it? I can make a beta release. |
I gave it a test, but got the following:
|
zha_gw = zha.get("zha_gateway", None) | ||
else: | ||
zha_gw = zha.gateway | ||
zha_gw = u.get_zha_gateway(hass) |
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.
In #252, I tried this as a first attempt as well, but ended up changing to getting the proxy object instead as it still had the hass
property whereas the new gateway that's in the new library no longer has any knowledge of HASS.
If you're moving to requiring 2024.8, then why not just change this to the proxy object:
zha_gw_proxy: ZHAGatewayProxy = zha.gateway_proxy
And then change to get access to the application_controller
to zha_gw_proxy.gateway.application_controller
everywhere?
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.
I am requiring 2024.8 mainly to avoid that those on older versions update..
As far as zha_gw_proxy.gateway.application_controller goes, it's preferable to refer to a function in utils.
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.
Yeah, the utils thing makes sense style wise. I was just getting at the fact that now it seems you have to access hass
and application_controller
from different objects.
@@ -50,6 +53,15 @@ | |||
MANIFEST: dict[str, str | list[str]] = {} | |||
|
|||
|
|||
def get_zha_gateway(hass: HomeAssistant) -> ZHAGateway: | |||
"""Get the ZHA gateway object.""" | |||
if parse_version(HA_VERSION) >= parse_version("2024.8"): |
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.
Why type check the version if in hacs.json
2024.8 is now required?
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.
To be able to restore backward compatibility. The requirement in hacs.json is to avoid updates for users that currently have something working on earlier versions.
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.
Oh, I think I see. So this will be set so that only those who are running 2024.8 receive an update notice via HACS, but will eventually be reverted post-release?
If so, that makes sense.
|
||
try: | ||
from homeassistant.components.zha import Gateway as ZHAGateway | ||
except ImportError: | ||
from homeassistant.components.zha.core.gateway import ZHAGateway | ||
|
||
from homeassistant.components import zha | ||
from homeassistant.components.zha import helpers as zha_helpers |
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.
These will fail to import if running before 2024.8, right? (The PR seems to code defensively around assuming the version may be less while also making a change to require it, so it's hard to see which direction it's going.)
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.
Thanks for the feedback.
zha and the helpers are stil valid in version before 2024.8 as far as I know, I'll test this on 2024.7.4 anyway.
I added some changes that fix it for me and opened a PR against the git remote add wbyoung [email protected]:wbyoung/zha-toolkit.git
git fetch wbyoung
git checkout dev
git reset --hard wbyoung/fix-253
git push origin dev I'm not sure what your process is. Hopefully the changes help you get something out that fixes the issue! |
@wbyoung I adapt to the context - maybe a cherry-pick is possible. FYI, I have this funcdtion to fetch a PR: $ declare -f gitpr
gitpr ()
{
local pr=$1;
git fetch origin "pull/$pr/head:pr$pr" && git checkout "pr$pr"
} |
Thanks! I see the following in the HACS UI:
|
I'ld call that a HACS bug, but I'll update the manifest. |
Yeah, probably. For what it's worth, I was thinking about this version field after seeing that error message. I looked it up in the HACS docs, and it just describes it as a minimum required version of HA to run the integration. Your prior explanation of changing the version made sense to me, but it's inconsistent with how I've seen (and personally used) this type of field before. I think it does make sense for anyone/everyone to get the upgrade even if they're not running 2024.8 because they could update HA at some point in the future, and it's nice to have all the extras already updated and possibly working. It may be annoying to update for something that doesn't change anything for your HA version, but I doubt everyone is reading release notes carefully. Also, HACS seems to make the same assumption in the UI based on that error message I saw. I'd be concerned as a user to see something like that and think that this integration was heading towards no longer supporting older versions of HA. Just some thoughts—feel free to ignore me! 😜 |
@wbyoung I agree - I also prefer to update components before the upgrade, and some do not allow that. |
No description provided.