xoofx / zio

A cross-platform abstract/virtual filesystem framework with many built-ins filesystems for .NET
BSD 2-Clause "Simplified" License
817 stars 61 forks source link

UPath questions #5

Closed mrvux closed 6 years ago

mrvux commented 6 years ago

Hello,

I was doing some integration tests to see if I could incorporate Zio in some of my projects, and while Testing, I could see that UPath(null) is allowed.

I was curious if there was any benefit for it (versus throwing a constructor exception) :

So allowing initial null input feels like a little bit like an exception just waiting to be thrown deeper in the software, and disallowing null path early would look like to help preventing.

Another short one (to stay in the topic), UPath to string implicits/explicits are reversed (in .net conventions).

UPath constructor can throw exception, so string -> UPath operator should be explicit, not implicit (implicit should guarantee that no exception not information loss will happen)

on the other hand, UPath to string could be implicit, as every UPath can be a string.

As a side note, I'm quite happy to PR those changes (eg : removing IsNull, and adding initial guard), but wanted to discuss them beforehand, just in case I missed something Thanks

xoofx commented 6 years ago

About disallowing null, I'm not in favor. Mostly because you can perfectly have a UPath as a member of a class, not initialized, accessed later, but actually the value is null. So you can't make any guarantee that the underlying string cannot be null (as you can't disallow the use of new UPath() alone)

About the conversion, in that case I care a bit more about practical than convention. The fact is that you want to use something like filesystem.WriteAllText("/temp.txt", "This is a content"); without having to explicit cast to UPath anytime you are passing a string (which is a common use case). If you forget about conventions, is it that annoying?

mrvux commented 6 years ago

Yes this is true UPath is a struct and not a class, so it's then possible to have uninitialised version lying around (and sealed class would not really be much better as would still need to validate for the class itself to be null anyway).

In that case I would only disallow null in case of FileExists(UPath path) (since Directory version throws exception), instead of returning false, in order to have the whole api consistent.

For conversion, it makes sense in that case indeed

It would still possible to have a string overload of "WriteAllText" (and others) that does explicit casting, but indeed that's a little extra work, and I don't see it giving so much extra value.

I tend to like the explicit cast, since it tells me I'm "doing something that can fail", so it's a good way to directly see that I should take care (specially 3 month later when reviewing the code), but I can live without it still.

xoofx commented 6 years ago

In that case I would only disallow null in case of FileExists(UPath path) (since Directory version throws exception), instead of returning false, in order to have the whole api consistent.

Actually, just pushed a fix so that DirectoryExists accepts null. The reason is that the underlying System.IO.File API is accepting null (returning false for both), did this for FileExists, but didn't check for DirectoryExists.

I tend to like the explicit cast, since it tells me I'm "doing something that can fail", so it's a good way to directly see that I should take care (specially 3 month later when reviewing the code), but I can live without it still.

I agree that usually an implicit cast (and actually, even an explicit) should not throw an exception. That was a tradeoff while designing the API, so that we have the guarantee that a UPath once instantiated is always valid while still having the implicit casts to play around to facilitate its usage. The implicit cast is "almost" a no-op if the string is a already a valid UPath.

mrvux commented 6 years ago

Since basic IO API accepts null, it's fine to have it in both cases then.

I started to integrate zio in one project, and seems to work fairly well, got a few other bits that could be thought of but they not relevant to this issue so I will post new ones and close that one.

Thanks