Don't fail os.remove(file) when the file doesn't exist

[iuliand-db]

This is what the docs say, and what os.remove.all does, but with a single file it may throw FileNotFoundException

[lefou]

According the the Documentation (readme.md) this behavior is correct.

A more appropriate fix is to correct the ScalaDoc.

[iuliand-db]

Could you point to the part of the README that says the file must exist? I checked again, and it only says that when removing a directory it needs to be empty, and otherwise use os.remove.all.

Given that os.remove.all does not complain if the top-level directory doesn't exist, it's less surprising to do the same for os.remove, especially since there's no alternative to "force" or delete only if it exists.

[lefou]

Could you point to the part of the README that says the file must exist? I checked again, and it only says that when removing a directory it needs to be empty, and otherwise use os.remove.all.

You're right, this is not stated explicitly in the documentation. But it's mirroring the behavior of Java File API as well as Unix rm command.

Given that os.remove.all does not complain if the top-level directory doesn't exist, it's less surprising to do the same for os.remove, especially since there's no alternative to "force" or delete only if it exists.

You can check for existence before you delete. After your proposed change, many other use cases may no longer work. E.g. you'll no longer see an error when a file, you expect to exist, doesn't. I'm not arguing which API is better, but I think we are already consistent with what users expect. Looks like what you really want is to mimic/add the force option?

[iuliand-db]

I'll stop after this comment, but my reasoning is as follows:

• the vast majority of users don't want their programs to crash because they tried to delete a file and the file was already non existant.
• the whole point of oslib is to offer a better API. If someone wanted the Java API, they'd probably stick to the Java API
• the analogy to the Unix syscall (or rm utility) is misleading. They both return a status code/error code, and it's up to the caller to check it or ignore it. If they don't care (as it usually is when removing a file), the program doesn't crash by default, the return value is simply dropped. However, Java made this return value be an exception, and coupled with Scala's lack of checked exceptions, leads to a crash when you'd rather ignore the error code.
• lastly, even if we accept the parallel above, os.remove.all implements -f implicitly (not just -r), while os.remove is simply rm. Symmetry (we can call it the principle of least surprise) suggest they both should force or not force together.
• check-then-act is inherently racy, and someone might delete the file between the test and the delete call

If there are some rare use cases where you'd need to know if the file you tried to delete, there could be a return value form remove that would indicate if the file was present or not. I would argue the default should be the most common case.

[lefou]

These are very convincing points. I agree, your proposal is very in the aim of usability. This is still a behavior-breaking change, so let's give others the chance to speak, whether they see any issue in this change. Thanks for your persistence.

I changed my mind, lots of valid arguments were presented

[lihaoyi]

I'm personally neutral:

• The workaround of using os.remove.all is easy enough, but it's also sloppy like using rm -rf in bash to remove single files.
• It's a breaking change, but I find it hard to imagine user callsites where the old behavior of crashing loudly is desired.
• OS-Lib aims to also provide "raw" access to the Java API without too much magic or helpers, in addition to the higher-level convenience methods, but the new inplementation is also just a direct call to the Java API just like the old one

Maybe we can merge this, and add an override to keep the old behavior around? Like os.remove(p, failIfNotFound = true), so those who want the opd behavior can get it, but by default people will get the new thing. This also has a benefit of providing a more complete coversge of the functionality exposed by the java filesystem API than we currently have, which is something that OS-Lib aims to do

[lihaoyi]

If we're going to do this, we should also consider changing the return type from Unit to Boolean, since that's the return type of the underlying deleteIfExists method. Alternately, if we don't want to break bincompat, we could introduce this as a new os.remove.ifExists method and leave the opd behavior in place

@lefou what do you think?

[sake92]

I'd also go with os.remove(p, checkExists = true) as a default, to preserve current behavior.

[iuliand-db]

Sounds good, and thank you for your flexibility, @lefou! I'll try to make the changes tomorrow.

[lefou]

I initially also liked the addition parameter, but found it hard to decide between checkExists vs. failIfNotFound. The latter is more explicit, the first feels shorter, but also leaves more room for interpretation what the consequences of a "check" might be. How about just force like in Linux rm command?

$ rm --help
Usage: rm [OPTION]... [FILE]...
Remove (unlink) the FILE(s).

  -f, --force           ignore nonexistent files and arguments, never prompt
...

I still struggle with the default value though. To really make programmers life easier, we should make the not-fail version the default, but that will semantically collide with the binary compatibility override, which I think we should provide in any case.
We could just bump the minor version and add a clear changelog entry and get away with it.

For completeness, we also have the option to add yet another named entry point remove.force(path), but without knowing the reasons, I don't like so much.

[sake92]

checkExists is consistent witb os.proc.call's check flag. Same meaning I guess. @lefou

[lihaoyi]

Yeah checkExists would match with os.proc.call's check flag, or request-scala's check flag. I think it fits the pattern nicely.

I'm willing to break binary compatibility here. We are already breaking bincompat in recent changes to Fansi and PPrint and Mill, and will need to do a coordinated release of the libraries anyway, so might as well include OS-Lib in that group. It's a tradeoff maintaining bincompat and maintaining a nice API, and since we have an opportunity to break compat presents itself we should take it.

In that case, I think the thing to do would be:

• Add a new parameter checkExists: Boolean = false to os.remove, change return type from Unit to Boolean.
• By default, we return true if a file was successfully deleted, and false if no file was found, and never throw an exception
• If checkExists = true, then we return true if a file was successfully deleted, and throw an exception if no file was found

[lefou]

Sound reasonable. Though, I don't think the way other projects handle their compatibility stories should hinder us doing the right thing. Changing bin-compat is no big issue, as long as we increasing the version properly.

[lihaoyi]

Superseded by #89 I think