Delay for triggering about filewatcher

[thomasfl]

Yes, let's do it!

[thomasfl]

👍

I think the best way to tackle this is to do what a similar js cli tool called watch does:

    --wait=<seconds>
        Duration, in seconds, that watching will be disabled
        after running <command>. Setting this option will
        throttle calls to <command> for the specified duration.

https://github.com/mikeal/watch

@gisikw You've been engaged in the same problem recently. Any comments?

If we add a --wait option then the --dontwait option should probably be renamed to --immediate to avoid confusion.

[AlexWayfer]

I support this decision!

[AlexWayfer]

@thomasfl I'd like to decide about the variables, maybe they need some changes with new behavior.

And today I thought of a slightly better scheme of work for --delay option, I'll create a new PR.

[AlexWayfer]

I'm looking at code… and I think that possible we don't need for wait option.

In Filewatcher#filesystem_updated? we're getting array of changes. And just run command (block) for each of these.

Maybe… ignore all changes except the first one? Or pass all changes (filename + event) as array… but should we run command many times for it?

I don't know…

[thomasfl]

Good idea @AlexWayfer! That's the simplest thing that could possible work, and probably also the solution that causes less surprises for the users.

If one file system scan finds more than one change, then just ignore all changes except the first one. Passing all changes as an array, can make sense if event is set to multiple_updates.

[gisikw]

If this change is going to be made, could it be behind a flag? Some use cases (like compiling-on-save) could be broken if only the first detected file is processed.

[AlexWayfer]

@gisikw Of course! Options are good 😅

But I don't know what default is better 🤔

Any thoughts, @thomasfl?

[thomasfl]

Agree with Kevin. The least surprising behaviour would probably be to have an option for only returning the first file if several changes have occured in one file system scan.

I don't have a good name for this option. Maybe --groupChanges / :groupChanges, since it makes several file system changes appear as a single event.

I did like the idea of returning a list of changed files.

[AlexWayfer]

@thomasfl Maybe --each? Or --every? But without short version, I guess, or -E.

Or --batch (-b).

[thomasfl]

Consider adding a --debounce action where users can specify the time in milleseconds without any changes before an event is triggered.

@gisikw @AlexWayfer

[AlexWayfer]

@thomasfl Why? I answered you here. No need for --wait, --delay or --debounce option. Just one command execution for many changed files once.

[thomasfl]

When many file changes happens, how do you know that all of the file changes is done? The debounce option specifies how long to wait before filewatcher knows that all changes are really finished. Just an idea.

[AlexWayfer]

Maybe just increase --interval? But you'll get less real-time command spawn, yes.

OK, this makes sense.

[gisikw]

FWIW, I think a --debounce option makes sense. Rolling things up into a single dispatch with multiple filenames also seems to resolve the issue, and the PR looks great.

That said, I think dispatching only the first $FILENAME is potentially misleading, and I know I could quite easily forget the -E flag, giving me a false sense that I was processing a complete set of files. Would much prefer this as an opt-in batching feature (or perhaps even don't yield $FILENAME, but a \n-delineated xargs-able list or something? Just to distinguish it from the standard behavior?)

@thomasfl To your point, I think @AlexWayfer's fix works just like your proposed debounce, provided that you also avail yourself of --interval?

[AlexWayfer]

I can make another PR with option, but I think --delay is better name than --debounce: we talk about delay between two command executes, that is greater than --interval, as I understood.

@gisikw I agree with multiple filenames in env variable, but what is better when -E is setted: $FILENAME with \nor $FILENAMES? The same about $EVENT, $BASENAME and others.

[gisikw]

Yeah, I'd argue for pluralizing the variables. Just nervous that shipping a new version of the tool, such that old CLI commands that were valid in prior versions don't explicitly break, but do fail to run on all files, makes me nervous for anybody who may not watch the changelog.

Pluralizing the vars would at least make sure that legacy syntax would fail (speaking for myself, I've got a bunch of tmuxinator projects with filewatcher commands hard-coded, so a same-syntax, different-behavior change could catch me flat-footed)

[AlexWayfer]

@gisikw Breaking changes must be shipped with new version, and dependencies must be locked.

I'll improve env variables in #53.

[thomasfl]

@gisikw Maybe this is bullshit, but what if both environments variables $FILENAME and $FILENAMES was exported? $FILENAME could be the first filename in the $FILENAMES list. Just to make the filewatcher CLI backwards compatible?

[AlexWayfer]

I've added --delay option in separate commit of #53 (now here).

Waiting for the decision about env variables.

[gisikw]

@thomasfl I would argue that you want it to break. Particularly since this is often used as a CLI tool, and people might not pay close attention to the version number if they add it on a new machine. My concern here is that the change would create a silent "failure", for example:

filewatcher '**/*.coffee' 'coffee -c $FILENAME'
# in another shell...
git checkout branch-with-a-few-changes

With the existing (and older) versions of filewatcher, this works as expected, and each file would get recompiled. But the PR changes the default behavior, and now only one of them gets compiled. It's a silent failure that the user isn't clued into, and they'd only encounter the need to change their scripts to use the -E flag if they just-so-happen to catch what's wrong under the right use case.

If the default behavior is going to change, then I think it's best to make things backward incompatible, because the same syntax now means something different, and you want to give users a very loud warning that they command might not being doing what they expect it to do.

That said, @AlexWayfer's --delay option would preserve the original behavior, it looks like, since the delay is default-off, which avoids the whole issue. Just my thoughts!

[thomasfl]

@gisikw Thanks for your feedback.

@AlexWayfer I have approved your pull request. Thank you very much for your effort.

I will need to test out the CLI before pushing it to rubygems. It can take a few days. I've been busy lately both at work and at home.

[thomasfl]

@AlexWayfer Good. Looking forward to seeing the new PR.

[AlexWayfer]

@thomasfl You merged it: #54

Maybe a new PR will be for the env variables after the final decision.