Define a CaseSensitivity type instead of using useCaseSensitiveFileNames bool #108
Conversation
jakebailey
requested review from
DanielRosenwasser,
andrewbranch,
gabritto and
EricCornelson
internal/tspath/path.go
Outdated
| func (o ComparePathsOptions) GetComparer() func(a, b string) int { | ||
| return stringutil.GetStringComparer(!o.UseCaseSensitiveFileNames) | ||
| return stringutil.GetStringComparer(!o.CaseSensitivity.IsCaseSensitive()) | ||
| } | ||
|
|
||
| func (o ComparePathsOptions) getEqualityComparer() func(a, b string) bool { | ||
| return stringutil.GetStringEqualityComparer(!o.UseCaseSensitiveFileNames) | ||
| return stringutil.GetStringEqualityComparer(!o.CaseSensitivity.IsCaseSensitive()) | ||
| } |
There was a problem hiding this comment.
I did not plumb this further, but maybe case sensitivity should be in stringutil.
internal/tspath/path.go
Outdated
| type CaseSensitivity uint8 | ||
|
|
||
| const ( | ||
| CaseSensitive CaseSensitivity = iota |
There was a problem hiding this comment.
This makes the default value "sensitive" which is backwards from before; will flip.
|
This doesn’t really feel necessary to me 🤷 I think |
|
The reason I'm not a fan is that case insensitivity is the "weird" case, yet it's the one we treat as the default of false. I have a really hard time remembering which sensitivity true or false is just by reading. |
|
I don’t think that’s right—Windows and macOS are case-insensitive, making the default of |
| assert.Equal(t, string(ToPath("file.ext", "/path/to", true /*useCaseSensitiveFileNames*/)), "/path/to/file.ext") | ||
| assert.Equal(t, string(ToPath("/path/to/../file.ext", "path/to", true /*useCaseSensitiveFileNames*/)), "/path/file.ext") | ||
| assert.Equal(t, string(ToPath("file.ext", "path/to", CaseInsensitive)), "path/to/file.ext") | ||
| assert.Equal(t, string(ToPath("file.ext", "/path/to", CaseSensitive)), "/path/to/file.ext") |
There was a problem hiding this comment.
this doesn't actually test the case sensitive case, right? since the path is all lowercase
There was a problem hiding this comment.
No, it doesn't seem like these are testing the case part. Probably this should have better tests.
It's not about the number of users (obviously Windows/macOS outshadows Linux for dev machines, though not CI), but about how often we need to special case things in our code. If we're doing something with the bool it's almost always a case of |
|
I guess it doesn't, since I didn't take it that far and reverse conditions. I guess my point isn't super great if you can see from the variable name what it is, but mainly when I was working on the FS stuff, I was having trouble writing tests where I couldn't remember what If this seems pointless, I can just drop it. |
|
I think the point is not so much just the naming, but more that when you pass the thing in, it's clear what you're passing in. If we have 2 boolean parameters in a row, you'll never have a hazardous refactoring. I personally think it's nice to have the safety of a distinct type. |
|
I'll reopen, but it's entirely possible that this would be moot as most places which set this flag by hand are already written... |
This feels less difficult to think about...