Closed philippgille closed 1 week ago
Hey @philippgille, thanks for the report!
Don't forget that all filters run concurrently. So calling ExecForEach(...)
starts that process happening in the background (the equivalent of running it with &
in the shell).
If you immediately call Error
, then you'll get the current error status of the pipe, which is probably nil
, and you won't then have any way of getting the results of the ExecForEach
, if you want them. Indeed, if the program exits, that process probably won't have a chance to run at all.
You could write instead something like this:
p := script.FindFiles(".").Match("coverage.txt").ExecForEach("rm ./{{.}}")
p.Wait()
return p.Error()
Thanks for the super quick reply!
Ah I see, Error()
is not a sink as the ones listed here, which is why it doesn't wait for the concurrent operations to finish.
I'd like to propose two things:
Error()
Godoc can be extended a bit to make this more clear, like to specifically say that it's not a sink that waits for the pipe to be finishedWait()
and Error()
behavior
Wait()
to this behavior is probably out of the question? Or maybe it's acceptable because code using it so far assumes no return value, and introducing a return value won't break that code.Wait()
could be kept as is, WaitError()
could be added, and potentially for consistency WaitIgnore()
could also be added.The great thing about script
is its conciseness, and being able to wait and get any errors in one line would be great!
If you agree, I could work on a PR.
Yep, it would be great to have a Wait method with returning error. This can allow us to write in one line:
if err := script.Exec("sleep 10").WaitError(); err != nil {
return err
}
I like this idea 😁
@bitfield do you need someone to work on this? I would be happy to contribute
Sure, go right ahead!
@bitfield PR for this issue is ready for review at https://github.com/bitfield/script/pull/211
@mahadzaryab1 the PR is great, but I wonder if we can really justify having two more or less identical methods, one that returns the error and one that doesn't. To play devil's advocate for a moment, if we already had WaitError
, what would be the point of adding Wait
?
After all, if Wait
returned error
, you'd be free to ignore it, as presumably existing programs do. But if you want the error value, you could receive it. Would it make more sense to have Wait
return error
instead?
@bitfield Sounds good to me! I'll make the change.
@bitfield updated https://github.com/bitfield/script/pull/211/ to have Wait
return error
instead.
@philippgille and @bcho, will this solution work for you?
That's great, thank you! 👍
Hello, I'm currently trying to migrate from some
build.sh
andbuild.ps1
files to a magefile usingscript
.Maybe I misunderstood the purpose of
Error()
, but when I don't need the output of a command (asString()
would return), I thoughtError()
would basically ignore the output, but still return the error. But it seems that when chained after anExecForEach
, the commands in that are not executed.For example, let's say we have files
coverage.txt
andfoo/coverage.txt
and I want to remove them. Then the following works:But the following doesn't:
Is that expected?