Small cleanups for recorder#68551
Conversation
- Fix comment - Fix incorrect typing - Add some more missing typing
|
Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration ( |
|
Looks like I can't just cleanup one without trigger an error on the others :( |
|
That didn't cascade as much as I thought it would. Didn't have to pull in anything close to the whole branch to make mypy in the CI happy 👍 |
| """Start processing events to save.""" | ||
| shutdown_task = object() | ||
| hass_started = concurrent.futures.Future() | ||
| hass_started: concurrent.futures.Future = concurrent.futures.Future() |
There was a problem hiding this comment.
Is this future threadsafe? It seems we're abusing the concurrent.futures.Future api. Some of it is only meant for use in executor implementations:
https://docs.python.org/3/library/concurrent.futures.html#concurrent.futures.Future
There was a problem hiding this comment.
That uses a threading.Condition under the hood for access so it should be thread-safe. While it is not broken AFAICT, it would probably be good to refactor the implementation at some point as I think there are better options available now.
There was a problem hiding this comment.
Well I was going to suggest using run_callback_threadsafe but that uses concurrent.futures.Future as well
There was a problem hiding this comment.
Looks like asyncio.run_coroutine_threadsafe does as well
There was a problem hiding this comment.
Either one is still probably a better option here since at least it means if something does change with it there will be less places to fix
There was a problem hiding this comment.
It's late late here so something to think about for tomorrow.
3 participants
Proposed change
Fix comment Cache newly written state attribute ids #68355 (review)
Fix incorrect typing
Add some more missing typing
Type of change
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: