script icon indicating copy to clipboard operation
script copied to clipboard

Ignore fs.WalkDir errors in FindFiles

Open parkerduckworth opened this issue 3 years ago • 2 comments

Updates FindFiles to use fs.WalkDir instead of filepath.Walk, and ignores any errors that occur during the search.

Closes #99

parkerduckworth avatar Jun 02 '22 21:06 parkerduckworth

Hi @parkerduckworth,

This new functionality does make a lot of sense from a pure UX point of view. However, it might ever so slightly violate the Principle of Least Surprise.

Why? You might ask. A number of reasons come to mind, for instance:

  • Hides errors...
  • ... which changes the existing behaviour of this function
  • Does not allow you to alter the behaviour, neither enable nor disable the new behaviour
  • Changes the assumption about empty results: was there an error (and if so, then what was it?), or the files or directories are simply missing?

The change we are adding claims that this new functionality aligns FindFiles() closer to how find (man 1 find) works by ignoring errors. However, one could argue that find does not ignore errors per se - it does not stop execution when it can't stat() (the system call) a file or directory; however, the errors are very much not ignored, as errors are printed to the standard error output and should there be even a single I/O error, then find would return a non-zero exit code once it finishes to let you know that there was something wrong during its execution. Additionally, find can return an error without any results, should the provided path be inaccessible, for example (perhaps not the best example):

Screenshot from 2022-07-09 21-14-04

This is something that countless people often rely on in their shell scripts. So, there is an API, and there is an interface between find and the user, of sorts, there. I, for one, would like to know that there was an error somewhere.

What do you think?

Perhaps a way forward here would be such where FindFiles() would collect errors it saw, so to speak, and then return an error type that carries details of multiple errors (see the hashicorp/go-multierror package). This would allow for the Error() function on the Pipe type (the fluent interface of Script that makes it so nice to use can also be a design challenge) to work as usual (since the error interface is still the one we return), and users would be able to reason about any potential errors.

A different approach would be to perhaps introduce a new function called Find(), which takes one argument that denotes the type of what to filter (akin to -type f or -type d when using find), and two other arguments - an error and results callbacks. Or perhaps only two arguments - a type and a callback (similarly to the existing Filter() function).

@bitfield, your thoughts? @parkerduckworth yours?

Thank you!

kwilczynski avatar Jul 09 '22 12:07 kwilczynski

Maybe a sensible compromise here is that:

  1. If FindFiles has any results, there is no error
  2. If FindFiles has no results, but there was at least one error, then that error becomes the pipe's error status.

What do you think?

bitfield avatar Aug 26 '22 14:08 bitfield