Follow up to porting run to cobra #1327
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
There was a problem hiding this comment.
I've marked this as fixship, I don't think we'll get more value of going back and forth on it. Making a call and codifying it with tests is a pretty reasonable state to be in; it constrains the behavior.
If you think it's a better decision to codify, I've written out what I believe the other option is.
| // Unix-like OSes will treat them as absolute and can be returned directly. | ||
| func platformSpecificAbsoluteUnixPathExpectation(cwd AbsolutePath, absoluteUnixPath string) AbsolutePath { | ||
| if runtime.GOOS == "windows" { | ||
| return cwd.Join(absoluteUnixPath) |
There was a problem hiding this comment.
This feels really surprising to me, but I don't know what to do about it. I guess it's a garbage-in, garbage-out scenario.
If we always know the cwd we could do something like this:
root := filepath.VolumeName(cwd) + string(os.PathSeparator)
return filepath.Join(root, absoluteUnixPath)I feel like this is more-likely to be correct, but either way we're making a guess as to what the user wants.
🤷♂️ I thought my opinions on this were stronger, but thinking more about it I don't know which I prefer. (It'd be exceedingly rare for a root-relative path to exist in the same place across OSes, and would have to be intentionally configured to be that way.)
There was a problem hiding this comment.
It's almost certainly user error for this situation to happen. Given that, I think the more windows-native thing to do would be to treat the unix path as a relative path in a \-delimited filesystem, as opposed to guessing at what the absolute path would look like.
It's possible there is a convention on windows that this is violating, but I'm not aware of one.
Addressing some of @nathanhammond's review comments from #1298
I added a test for
AbsolutePathVarso that we can have a way to discuss the intended behavior.