Closed krader1961 closed 1 year ago
I don't see a good reason to deviate from Go, which has os.Remove
(non-recursive) and os.RemoveAll
(recursive). It's not clear why you reimplemented recursive removal instead of using os.RemoveAll
.
I feel &ignore-missing
is better named &missing-ok
(there's precedent in *[nomatch-ok]
). It is not exactly ambiguous, but it requires a bit of thinking to deduce what "ignore" exactly entails (in this case, ignoring any error from missing files).
I also feel this command shouldn't live in path
. It's my fault for letting in is-dir
, is-regular
, temp-dir
and temp-file
, but I'd like to make path
just about manipulating paths themselves, and create a new os
module to mirror Go's os
package. But feel free to leave it there and I'll do the cleanup after merging.
I mostly agree with your feedback, @xiaq. I need to think about whether a new os
module is preferable. On the one hand I am inclined to agree given issue #1659 that I opened. I even noted this tension on where such commands should live in that issue. The question is how tightly we want to bind Elvish builtins to the Go package that underlies their functionality? This obviously has implications for the path:stat
command I want to see merged. Should that also be named os:stat
?
I don't see a good reason to deviate from Go, which has os.Remove (non-recursive) and os.RemoveAll (recursive). It's not clear why you reimplemented recursive removal instead of using os.RemoveAll.
Because the Go os.RemoveAll
documentation says this:
RemoveAll removes path and any children it contains. It removes everything it can but returns the first error it encounters. If the path does not exist, RemoveAll returns nil (no error). If there is an error, it will be of type *PathError.
It is not clear that is the preferable behavior to expose since it elides (hides) some errors. My implementation exposes all errors. I also haven't tested the os.RemoveAll
behavior. Does it actually delete all files that can be deleted even if there is an error deleting a particular path? The documentation of os.RemoveAll
is ambiguous on that point.
I don't see a good reason to deviate from Go, which has os.Remove (non-recursive) and os.RemoveAll (recursive). It's not clear why you reimplemented recursive removal instead of using os.RemoveAll.
The main problem with using os.RemoveAll
is that it silently ignores a non-existent path. Which we only want to do if the &missing-ok
(or &ignore-missing
) is true. So using that Go function requires adding an explicit test whether the path exists. It is simpler to just use os.Remove
and recurse if the path is a non-empty directory. Not to mention aggregating removal errors which os.RemoveAll
makes possible while os.Remove
does not.
I am going to close this P.R. since the implementation is moving from the path
module to a new os
module in light of previous feedback. The new P.R. will include a comment why the Go os.RemoveAll
is not used.
I considered adhering more closely to the traditional Unix
rm
andrmdir
commands. For example, by implementing each independently and supporting the--force
option. However, I concluded there are insufficient reasons to do so. In particular, the POSIX--force
(or-f
) option is most often used solely for its side-effect of making elimination of a non-existent path a non-error. I also don't like that-f
also fixes permission errors to allow allow removing files. If we decide that feature is needed it should be added under a distinct option (e.g.&fix-perms
).Related: #1659