Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implicit string conversion to/from UPath is subtle & error prone #75

Open
agocke opened this issue Mar 8, 2023 · 3 comments
Open

Implicit string conversion to/from UPath is subtle & error prone #75

agocke opened this issue Mar 8, 2023 · 3 comments

Comments

@agocke
Copy link
Contributor

agocke commented Mar 8, 2023

Just hit this:

var fs = new SubFileSystem(new PhysicalFileSystem(), realPath));

I assume the proper way to do this is:

var physicalFs = new PhysicalFileSystem();
var fs = new SubFileSystem(physicalFs, physicalFs.ConvertPathFromInternal(realPath)));

If there weren't an implicit conversion, I probably would have guessed that. Overall, it seems like keeping UPaths and strings a bit separate is a good idea. Especially when working with subpaths like this, the chances that you'll confuse one for the other is pretty high.

@xoofx
Copy link
Owner

xoofx commented Mar 8, 2023

Agreed, that's one of the gray area of the API. Not allowing implicit conversion from string to UPath would have lead to annoying explicit cast in many places (more than the creation of filesystems but all methods provided by IFileSystem) so made this choice of simplicity vs correctness, but the downside is that there are a few cases like the one you point out that are not protected against a misusages (and the XML doc is not saying anything about this). Not sure I would want to change this after all these years this API has been released though...

@agocke
Copy link
Contributor Author

agocke commented Mar 8, 2023

An analyzer might work here, along with an option to enable it. Not taking a breaking change makes sense.

In the meantime I might try an expedient route and see if the BannedApiAnalyzer could catch this.

@agocke
Copy link
Contributor Author

agocke commented Mar 8, 2023

Aside: the / operator override is clever. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants