search

lucasmeijer / NiceIO

For when you've had to use System.IO one time too many
MIT License
128 stars 19 forks source link

RelativeTo now supports non-child path cases (case 867613) #8

Closed mrvoorhe closed 7 years ago

mrvoorhe commented 7 years ago
lucasmeijer commented 7 years ago

Hey Mike,

Depth -> looks good

SameVolumeAs seems like a too narrow usecase to warrant an api entry point for. how about we just expose .Volume

the RelativeTo changes i'm not quite sure what to do with. can you elaborate a bit on why we need those? The part i'm not so happy with is that we allow ".." to be an element in the path. I hope we can avoid having to do it, but maybe you have a usecase that can only be solved like this. I'd love to understand that usecase before taking the .. changes though.

lucasmeijer commented 7 years ago

actually if we expose .Volume, we should change the fortunatelly private, uglier name of _driveLetter to be _volume as well

mrvoorhe commented 7 years ago

Regarding SameVolumeAs, originally I actually had exactly what you suggest, although I simply called it DriveLetter after the field name. My think for switching to SameVolumeAs was

So that was my thinking. I don't feel strongly about it. If you would prefer I go back to the Volume property I can do that, or now that I'm thinking about it more, I'm leaning toward just doing a _driveLetter compare inside RelativeTo implementation. A Volume property feels like a Windows'ism and I think we should keep OS specific stuff off the public API unless there's a real need for something.

the RelativeTo changes i'm not quite sure what to do with. can you elaborate a bit on why we need those? I will look into it in more detail tomorrow and get back to you. What I know off the top of my head is that people internally & external have been hitting an exception thrown by the if(!IsChildOf) check in certain cases. It's probably some borderline to lax path handling code. Zilys and Levi thought it seemed like it was a valid case to handle. Once they pointed it out, I thought it seemed harmless enough to allow.

The part i'm not so happy with is that we allow ".." to be an element in the path I thought ".." was already an accepted & valid path element. Is that not the case?

lucasmeijer commented 7 years ago

I don't think we have to try really hard to "hide windoism". npath paths can have a driveletter, which is a feature we need on windows, so I have no problem with showing this in the api. I think your "Volume" name is more elegant than my "driveLetter" though. let's catch up on slack maybe on what to do with "..". I'm not saying we should absolutely not do it, but I would really like to understand the usecase before we do

mrvoorhe commented 7 years ago

@lucasmeijer I was working on Friday's are for fun stuff on Friday, so I didn't get a chance to look at the bug. I will do that today and then touch base with you on what the use case is.

mrvoorhe commented 7 years ago

@lucasmeijer The motivation for the change to RelativeTo is over in the devs-il2cpp-core chat, but I'll summarize here in case we need it for reference.

There was a bug/issue with the emscripten tools and long paths. We needed to do relative paths in the response file

And while tracking down that, I noticed that the MsvcToolChain is using RelativeTo, only it has an if IsChildOf guard around it. I don't think this RelativeTo usage is as critical as the Emscripten case, but it does seem to indicate that there is a desire to use RelativeTo in situations beyond just the IsChildOf case.