fix: filter file watcher duplicate events #200
Conversation
|
Seems to be failing on mac, but all the file watching logic is notorious for platform specific quirks. I'll go ahead and re-run the failed jobs, but I doubt they'll pass this time (more so for my sanity). I won't be able to take too close of a look till after holidays, but I'll still be around to hit the CI button or for reviews (I also think CI l still runs in your fork when you push to it for whenever I'm not around) |
|
Alright no problem. I had a feeling it would only work on my machine, it's good you have a CI to check that. I'll fix them. It does run on my fork after enabling it. |
|
Great! And do note that the watcher tests are a bit weird. They're flaky when using short delays, but longer delays suck for dev-feedback loops, so it starts with shorter delays and retries the tests a few times with increased delays on failure Just in case the logs for the failed tests look confusing |
|
Alright good to know, I'm currently looking at the tests and I'm suspecting the delay is too short. |
|
Found this issue thread. Looks like it could be relevant (specifically that write events may be getting lost when using the debouncer on macos) |
|
I just pushed a commit that should fix the Aside: trace logs are currently getting filtered out from tests, but I'm starting to think that it'd be a good idea to retain them still |
|
Eh went ahead and changed that too. We really need all the info we can get when it comes to CI-driven development 🙃 |
Yep, I agree.
No problem go ahead.
From notify_debouncer_full docs:
Can confirm it still works, happy to see this merged then :) |
|
I want to offer a huge thanks for all your help on this :) |
|
You're welcome, I'm glad I could help. This is a great tool that I really love and want to help with and see grow (or at least not abandoned). |
|
There are plenty of things to work on to say the least. I can tag you in some issues that may be easier to start out with |
|
I've seen a few already, but I wouldn't mind if you tagged the good first issues. |
|
Keep in mind that features don't have to be perfect to open a PR. I'm more than happy to help with draft PRs. From looking over the issues I see:
I picked those because they're either well tested or generally well isolated I also have a lot of other things in an obsidian notebook, so I can start opening issues for those over the next few days. Feel free to reach out in whichever issues you want to take on so that we can talk about the general design/approach |
|
Opened some new issues that include a lot of potentially good starter issues along with actually labeling easy(ish) issues https://github.com/trimental/inlyne/issues?q=is%3Aopen+is%3Aissue+label%3AE-easy |
This fixes #198
As discussed in the issue, I refactored the file watcher to use notify_debouncer_full.
I did so with as few modifications as possible to avoid breaking anything. The most interesting part is the
DebounceEventHandlerimpl in which I basically filtered unwanted and duplicate events.The debouncer timeout of 10ms seems good for up to 100 ~ 150Kb files on my machine.
All unit tests passed locally. I've also tested with the files I was having issues with.
I also added logs I found interesting for debugging and changed the log format to show milliseconds since the file_watcher is always faster than a second.