Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add optional event debouncing for auto-restart #940
Add optional event debouncing for auto-restart #940
Changes from 8 commits
7d441d9
59b20fd
d3f258a
ab3d33a
7493cf5
03039e0
e46f162
c70a721
2ad37b5
de0df29
032697d
b064b47
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 do we need to ensure that it runs only once? It seems like it should behave OK even if called multiple times.
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.
Here's a different implementation, like I did it in MkDocs.
It has the benefit of being instantly stoppable, rather than having a 0 - 0.2 sec delay before being able to stop.
It also ensures to instantly stop firing events when it's stopped, maybe eliminating the need for
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 this @oprypin!
Using a condition like this is indeed better for instant stopping, I'll work that into the implementation.
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.
@taleinat Well you can just "accept suggestion", I think that will also end up simpler than, for example, keeping it a queue. It's a fully functioning replacement, just in 1 of the new tests will need to add a microscopic sleep to make it pass.
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, and your work to make it such a drop-in replacement was helpful and is appreciated!
This version however does not handle well the case where a shutdown signal is given during the debouncing interval. Also, the use of
Condition.wait_for()
is more complex than necessary.So instead I've integrated the approach while addressing these two concerns.