-
-
Notifications
You must be signed in to change notification settings - Fork 34.3k
Fix blocking code in AirNow sensor #117615
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
Conversation
Hey there @asymworks, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
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.
LGTM
I started adding a new async helper in #117721 that can be used for cases where we want to load uncached ZoneInfo in the event loop |
Another option would be to use https://dateutil.readthedocs.io/en/stable/parser.html but it might need to parse in the executor |
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.
Hi there @jnewland 👋
Could you please git rebase this PR and adapt it to the changes listed above by bdraco?
Thanks! 👍
../Frenck
@@ -36,7 +36,7 @@ | |||
"name": "[%key:component::sensor::entity_component::ozone::name%]" | |||
}, | |||
"station": { | |||
"name": "Reporting station", | |||
"name": "Reporting Station", |
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.
This change is unrelated and not correct either.
"name": "Reporting Station", | |
"name": "Reporting station", |
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
@bdraco @frenck If we use |
Proposed change
Fixed warning of blocking in AirNow sensor creation (because of a ZoneInfo call). This was actually not doing anything because AirNow returns timezones as three-letter abbreviations (i.e. CST), but ZoneInfo only works with IANA strings (i.e. "America/Chicago") because the three-letter abbreviations are non-unique. AirNow is a US based service, so the three-letter works for it, but there isn't a good way of parsing the three-letter ones, I don't think.
So, my solution is just to treat the time of observation as a string with the three-letter zone, and not bother with actually parsing the time into an actual datetime object.
I also added the state to the reporting station sensor.
In a future PR, should we think about moving more things to sensors? Dev docs say to avoid using extra_attributes for things that change (like the category and category level in this case, perhaps even observation time).
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: