Add file throttling #34
Conversation
lib/cortex/file_watcher.ex
Outdated
| GenServer.cast(Controller, {:file_changed, file_type(path), path}) | ||
| %State{file_events: file_events, throttle_timer: throttle_timer} = state | ||
|
|
||
| unless throttle_timer do |
There was a problem hiding this comment.
hmm, maybe I'm missing something, but does this ever get set to anything other than nil?
There was a problem hiding this comment.
Nope, that definitely looks like an oversight. Will fix it up in a bit 👍
There was a problem hiding this comment.
Okay, code is updated to actually set and clear the timer.
axelson
force-pushed
the
add-file-throttling
branch
from
d70ffcf to
5bfced7
Compare
Ideally we could make use of `Kernel.ParallelCompiler.compile/2` but that would require larger changes to code.
axelson
force-pushed
the
add-file-throttling
branch
from
5bfced7 to
da0100f
Compare
lib/cortex/file_watcher.ex
Outdated
| %State{file_events: file_events, throttle_timer: throttle_timer} = state | ||
|
|
||
| throttle_timer = | ||
| unless throttle_timer do |
There was a problem hiding this comment.
won't this set the throttle timer to nil the second time the file event is run?
There was a problem hiding this comment.
Once again you are completely correct. Sorry for the back and forth but I have updated the code to fix this issue.
To help prevent us (well me) from making issues like this in the future it would be nice to add some tests. As part of this change I did write a test, but in my opinion it is too tied to the current implementation (liberal use of :sys.get_state) to be a worthwhile test going forward. In order to better test the module it is necessary to change the implementation slightly to facilitate testing, specifically I think the start_link function could take an optional argument that would store both the file_changed recipient (defaulting to Cortex.Controller) and the throttle_timeout (defaulting to 100). I'll just put that up as a separate PR.
axelson
force-pushed
the
add-file-throttling
branch
from
02901d8 to
675dd77
Compare
|
@glittershark can you take another look? |
|
Thanks! |
2 participants
Adds a simple throttle timer to group changes together and avoid tests running multiple times due to multiple file events being emitted by one editor save operation.
Fixes #9
Code inspired by #21
Code based on #33 (to keep the formatting changes separate)