Reset fileset should merge changes on top of source files (fixes #330)

[micha]

No description provided.

[martinklepsch]

@micha what will happen if the user modifies the original file? I.e. some tasks modify x then watch task picks it up, does stuff. Now the user changes x again. This would override the task-modified version of x right?

[micha]

@martinklepsch yep, and this is correct behavior i think, because if you want to modify those files incrementally you'd put the associated tasks after the watch task in the pipeline.

[martinklepsch]

Hm. The alternative would be to have the task-modified versions take precedence which would also make sense in a way because the watch task watches things "after" those tasks?

[micha]

The task-modified versions do take precedence with this patch. The bug that was reported was that task-modified versions were being overwritten by the user files.

[micha]

This is all kind of a bikeshed too, because why make things complicated? Just put the watch task first in the pipeline and avoid all these complications.

[martinklepsch]

I think we misunderstood then but everything is clear now.

I think it makes sense to have tasks that are only required to run once outside of the watch loop. Less overhead for incremental compilation cycles. That said probably tasks like cljs-repl and reload could also be optimized further to not re-read/re-write the edn files every cycle.

[martinklepsch]

👍 🚀

[micha]

Yeah all tasks should be caching their own results and doing a diff on the fileset to avoid doing unnecessary work. That said, the cljs-repl task is only writing this one file which is adding maybe a few microseconds to the build time. I guess it's more of a problem that people see the task saying it's doing things and they think that they can make things faster by optimizing their pipeline, which isn't really the case. So we should probably have the tasks not rewrite those files just to avoid confusing the user.

[pandeiro]

@micha The reason I put cljs-repl before watch is actually because I am wrapping cljs-repl in an all-purpose, configurable REPL task, and I thought it would be best if that nREPL server is launched "first thing", before the watch and the rest of the pipeline.

It's not a big deal if "that's not the way to do it" (in my case I just put it after), but how will users know that?

[micha]

@pandeiro I wasn't trying to say "that's not the way to do it", sorry if it looked that way :)

Is there any reason why your task isn't compatible with being called again by the watcher?

[martinklepsch]

@pandeiro I think the behavior you reported was a bug and thus has been changed to preserve the file as it has been changed by tasks in the pipeline before watch. While putting it after watch is probably the most robust way the other ways are equally valid imho. You just have to know what you're doing a bit more.

[pandeiro]

@micha Nope it totally can be put after watch, I thought it might delay the availability of the nREPL server a few instants more than necessary -- that was my main thinking in putting it before. @martinklepsch "You have to know what you're doing a bit more" -- in this case, do you know if there any additional caveats I'm unaware of, now that watch accounts for previous changes to the fileset?

[martinklepsch]

@pandeiro If a task before watch modifies a file this modified version will be passed to any further invocations in the watch loop no matter if you change this file in your source dirs. This is the only caveat as far as I'm aware/understand.

[micha]

Yep, I think that's the situation. Like in your case, the main.cljs.edn file is processed by the repl task before the watch task is called. This patch ensures that the version of the file produced by the task overwrites the original main.cljs.edn file in your source paths each time the rest of the pipeline runs. Now if you change main.cljs.edn the changes won't be seen by tasks after the watch task, because the changes are being overwritten by the version produced by the repl task.

This is logical and correct, because there is no way for the watcher task to know that it should rerun things that come before it, like the repl task, to accomodate changes to main.cljs.edn in your source files.

So the reason why you wouldn't want to put the repl task in front of the watch task is to eliminate the need for the user to know the relationship between main.cljs.edn and the repl task. Basically, if you put tasks in front of the watch task you're fixing those files at that point in time, and that might have other consequences that are not readily apparent to the user.