Ignore irrelevant DidChangeWatchedFiles notifications
#534
Conversation
src/actions/notifications.rs
Outdated
| // ignore irrelevant files from more spammy clients | ||
| if !params.changes.iter().any(|change| { | ||
| change.uri.as_str().ends_with("/Cargo.toml") || | ||
| change.uri.as_str().ends_with("/Cargo.lock") || |
There was a problem hiding this comment.
I'm a bit worried about having these strings hard-coded here, and the corresponding patterns at https://github.com/rust-lang-nursery/rls/blob/master/src/actions/notifications.rs#L46, it seems like it would be an easy mistake to change one without the other. I would like them to come from a single source of truth. I think the easiest solution is to have an object or type with methods to provide the data.
There was a problem hiding this comment.
Yep that's probably a good shout, I'll take another look at it when I get time
|
I've bound the watcher & not relevant filter together in The filter logic matches the glob requested. It should be obvious to keep them inline when the globs change (ie for workspaces). |
src/actions/mod.rs
Outdated
|
|
||
| let local = &path[self.project_uri.len()..]; | ||
|
|
||
| local == "/Cargo.lock" || local == "Cargo.toml" |
There was a problem hiding this comment.
Thanks I'll fix that
| } | ||
|
|
||
| impl<'ctx> FileWatch<'ctx> { | ||
| pub fn new(ctx: &'ctx InitActionContext) -> Self { |
There was a problem hiding this comment.
I think it would be simpler to make FileWatch a module and make the two interesting methods functions which take ctx as an arg.
There was a problem hiding this comment.
This design allows fast calls of is_relevant. Notifications can include hundreds of thousands of file change references, so it makes sense to avoid allocations in the loop.
There was a problem hiding this comment.
OK, good point! Could you leave a comment about that somewhere please?
There was a problem hiding this comment.
Sure, I've added a note to the is_relevant method.
Add implementation note Cleanup `is_relevant` usage
|
|
||
| let local = &path[self.project_uri.len()..]; | ||
|
|
||
| local == "/Cargo.lock" || local == "/Cargo.toml" |
There was a problem hiding this comment.
While I understand that changing Cargo.{lock, toml} files should warrant a Cargo rebuild, I think it's also reasonable not to ignore any *.rs file here, even at risk of being a bit over-eager. I think it's safe to do here and this can happen a couple of times, e.g. when files are changed via source control or when a quick fix might've been applied outside current editor.
Note to self: previous eager rebuilds on every change might've triggered unnecessary Cargo rebuilds due to build scripts and similar, so maybe it's important to ignore build script-emitted files.
While it'd be nice to still watch *.rs files, I think we can't reliably do that unless we analyze what changed externally and what changed via build, so disregard what I said and I'll just leave it here to have that in mind.
|
Thank you! |
A big improvement for atom-ide-rust.
Resolves #531