-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
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
Bump pyvlx to 0.2.26 #115483
Bump pyvlx to 0.2.26 #115483
Conversation
Hey there @Julius2342, @DeerMaximum, 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.
Please add a reference to the changes made since the previous library version
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Its a bit unexpected that a task would be created in Julius2342/pyvlx@0.2.21...0.2.23#diff-4ad3fa98aaad618218e4216ebc26247b1b2a20b5ba8e077c57808bf5b493c291R121 as normally it would be awaited instead |
It looks like this library does a lot of fire and forget tasks without holding a strong reference to the tasks https://docs.python.org/3/library/asyncio-task.html#asyncio.create_task
|
Yes, I worked on a modification on pyvlx to gather all tasks... However at the moment I don't find the time to complete.. The PR for pyvlx is there, but still on a draft state.. Need to modify some module tests to make them final. |
@pawlizio I'm not completely sure, but is this patch to move your component https://github.com/pawlizio/my_velux to the Velux integration of Home Assistant? |
Could we bump to version 0.2.25 instead? This would fix an issue with a specific cover type for some users. |
@pawlizio It looks like things related to the comments in this PR have been addressed in v0.2.24 of pyvlx. In addition to that I have added a missing component in v.0.2.25 that a few people depend on. Could we please adapt this PR to bump pyvlx to v0.2.25? Or do you still see things that are missing to get it accepted ? |
@bdraco The fire-and-forget issue is now solved. In 0.2.25 the tasks are awaited when pyvlx will be disconnected: https://github.com/Julius2342/pyvlx/blob/912876029cd64cf361cebee9413bd6efd85b1e29/pyvlx/pyvlx.py#L109-L110 |
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.
It looks like nothing ever cleans up the tasks when they are completed so it will leak memory.
If you make self.tasks
a set
you can do task.add_done_callback(self.tasks.remove)
after added the task to ensure its removed from the set once its done.
Example:
Line 840 in 80dbce1
task.add_done_callback(self._tasks.remove) |
@bdraco Thanks for the hint, I considered this in pyvlx 0.2.26 now |
Thanks @pawlizio |
Proposed change
Bump pyvlx to 0.2.26 as preparation for PR #112451
Major changes:
Full change log:
Julius2342/pyvlx@0.2.21...0.2.26
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: