DSL "watch" method is misleading for both users and plugin developers

[e2]

Overall, this is about handling modifications from editors.

The issues:

1. when saving a file (e.g. vim), watch is called multiple times - which makes the method useless for users, since the parameters (the files) don't say for which handler (modify, add, remove) the block is called. (If there was a parameter passed to the watch block which saying for which handler the block was called, a case or switch could be used to avoid writing a plugin just to handle only run_on_modifications or adding a callback and filtering files yourself).
2. it's not obvious for plugin developers that because editors can trigger all 3 events, they should not actually perform any actions within the watch block - and instead run them in the handler(s). (guard-rspec and guard-cucumber actually do this correctly, but that's not reflected neither in the guard README or dsl.rb documentation).
3. I've wasted my share of hours trying to work out why actions are triggered multiple times. I'm sure many, many other people have.
4. "watching" a file and "doing" something when it's modified is basic expected functionality. Sure, it's not up to guard to handle editors, but you shouldn't require ugly hacks or writing your own plugin for the user to add handling something you'd expect guard to cleanly allow out of the box. Without rocket science.

guard-shell is a perfect example of how easy it is to misuse the watch() method.

hawx/guard-shell#11 (comment)

(My ugly hack shows the need to cleanly be able to run an action once - only for modification.)

Ideas/solutions: add a parameter for watch and mentioning it in the README (with the typical editor example), so people can at least limit the calls from within the dsl and work out what's wrong there - without having to turn on logging debug options, etc.

[thibaudgg]

Hi @e2, first thanks for you detailed issue.

I agree adding an event params to watch method could helps here. I'ld give:

def watch(pattern, event, &action) where event could be mofidied | added | removed

Keeping back-comptatibility should be quite easy here.

@rymai what do you think of that?
@e2 in the meantime could you propose a PR to improve the README? Thanks!!

[e2]

I'm not really sure - the issue seems complex. Especially since it depends on the "goals" of Guard and the usage cases for which it is most optimized. I'm wondering how often the watch() method is used ONLY for editing - in which case the user doesn't care if it was edited in place or created or moved as part of a atomic save.

Personally, I'd prefer to have a "when_edited" method or "when_content_changed" - to hide the abstraction of whatever editor or tool does to the file. And not call the block 3 times.

I think what I personally want to watch (most of the time) is the content of a file, not the file itself. If I don't care what the editor does to change the file, neither would I want to express those details in the Guardfile.

Maybe a "watch_content" would be good? (And for plugins that would trigger only the run_on_modifications callback anyway - guard-rspec and guard-cucumber only really care about that single event type anyway).

[e2]

I've realized the use case I need is just to know if "something changed" and then "run something".

This means that for me, adding a file is no different from modifying a file...

... which means either callback for "added" or "modified" should be called, but not both!

There is no "clean" way to distinguish between a file that was added (from scratch) or a file that was "added" because it was edited (the former causes one callback, the later would trigger two).

(If fixed, I think this would help with e.g. guard-rspec, because saving a new files would trigger tests - currently, AFAIK the "added" callback is ignored because it's so clumsy to handle added files without having a double-run on edited files).

To handle this, we could have:

watch('some_file.txt', :added) { "same action for some_file.txt" }
watch('some_file.txt', :modified) { "same action for some_file.txt" }

But besides having multiple watches for the same file (AFAIK currently only the first match is triggered), and the code duplication, adding an array instead:

watch('some_file.txt', [:added, :modified] ) { "same action for some_file.txt" }

doesn't make it obvious that it's an "or" condition (single callback) and not an "and" one (separate callbacks).

We could pass parameters to the block:

watch('some_file.txt' ) { |event| "same action for some_file.txt" }

... but then again, the user can't have the the callback run once for either event, but not both.

So this suggests adding a new event, e.g.: "added_or_modified" - which basically reflects what I mentioned earlier "content_modified" (whether "added" or "modified" - I don't care, as long as it's just run once).

Personally, I'd rather have:

watch_content('some_file.txt') { "same action for some_file.txt" }

and an extra callback to make things clear:

def modified_content(result)
  # This is always called only once, no matter if the file was added, modified or deleted
end

This means even "delete" would trigger the same behavior as just saving an empty file (meaning: "content was changed".

Besides, any error handling related to the existence or non-existence of files is handled by plugins and tools separately - I can't think of a use case where users strongly expect guard to distinguish between empty files and deleted ones - and need the callback to work as it is now.

So if both emptying a file and deleting it caused the exact same handler to be called, the user could distinguish themselves which (e.g. by using File.exists?). And if someone REALLY wanted something more specific than "file contents changed", that's what the listen gem is for...

Summary: I'm suggesting guard moves from "tracking files" to "tracking content" in it's ideology, since it's a higher-level tool.

[thibaudgg]

Summary: I'm suggesting guard moves from "tracking files" to "tracking content" in it's ideology, since it's a higher-level tool.

I think it could be a good direction to take.

I propose to wait on @rymai feedback (on vacation, like me, I guess).

[rymai]

Hi guys (I was not in vacation, only working on other stuff),

First, thank you @e2 for your involvement!

Second, I'd like to clarify something about hawx/guard-shell#11:

Here's what currently happening (e.g. after editing a file with vim):

1. watch() block is called 3 times because the edited file triggers all 3 callbacks (modification, create, remove) with the block
2. the result of the of the block is passed to every handler, but you're handling only run_on_modifications

This is clearly a bug in Guard's implementation, Guard::Watcher.match_files should not call the watchers's blocks here, they should be called only if the plugin implements the right method.

This will ensure that 1 event => 1 watch block call => 1 handler call.

Thirdly, I agree that Guard, as a higher-level tool should report generic content modification, not real FS events. That's something we can definitely improve. I suggest to set this as the main goal for Guard 3.
I don't have enough perspective right now to give you an opinion on your proposals, but let's discuss this matter in a new issue that we could name: "Guard 3: proposals for a less surprising tool!". ^^

One last thing, I'm not sure we should/have to change the watch DSL (e.g. for watch_content), we could "just" fix its implementation to be less surprising... Don't you think?

[e2]

I've got things working, so after some cleanup I should have a pull request good enough for reviewing.

Basically, I'm "reducing" the low level notifications into a "single" and "reasonable" notification per file, which means files will get only one callback anyway - so the high-level bug you described, @rymai, probably won't matter anyway (at least on Linux as far as I've tested).

I'm handling stuff like :

mv foo bar && mv bar foo

which now triggers just a single "modified" for 'foo' (given only foo is being watched here) and no "added" and no "deleted" callbacks are triggers.

(NOTE: making this example a noop wouldn't make sense, because it's the equivalent of 'touch', which people expect to trigger a change notification).

And I got things working like:

touch foo && rm foo && touch foo && rm foo

which is now a noop (no callback called).

Or, just handling what my Vim does (with patchmode and on ecryptfs):

:modified -> :removed -> :added -> :modified

(Which is now correctly interpreted and triggers a single "modified" callback and nothing else.)

No more horrors of adding a new file and having the tests run twice ... or not at all!.

I've also added some tests, so by using a tool (like inotifywait -m) and gathering the specific sequence of events, support for strange editor behavior can be added as unit tests.

This includes easily adding rare scenarios, like the lack of proper timestamp handling in ecryptfs - which makes almost every editor behaves a little differently on my system.

(Editor behavior is not only system/fs adapter specific - it's more case-by-case than that, as my ecryptfs proved to me).

And this is all properly handled through the TCP listener as well - all with minimal code changes and (hopefully) without breaking backward compatibility. (I have a few very ugly hacks to remove first to make sure).

So there shouldn't be any more dreaded "multiple runs" of tests after saving a file or juggling with editor options just so guard works as intended. Hopefully.

[rymai]

That's an awesome news @e2! Can't wait to see the PR! :)

[thibaudgg]

😍

[e2]

Since there are so many interconnected approaches and issues (between guard and listen), I've created these:

• Guard3 API feature request
• Listen3 API feature request

[thibaudgg]

Guard & Listen are already in version 2.x, so it should be Guard3 & Listen3 API right? :)

[e2]

I was thinking about a 2.0 of the API - but you're right.