-
Notifications
You must be signed in to change notification settings - Fork 452
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
Remove uses of pathtype library. #677
base: main
Are you sure you want to change the base?
Conversation
5537ca9
to
19a613f
Compare
`pathtype` was not a great success for us: - it did not catch any bugs other than exposing some odd behavior in `readProjectFromPaths`; - it baffled everyone who hadn't spent hours staring into its API (that is to say, me) - it used `error` to check string literals, which is... fine, I guess, but doesn't help much in actuality; - it complicated error reporting and assemblage; - completely switching away from `FilePath` was not an option, as libraries like `directory-tree` and `Glob` require it; - its documentation is very poor and difficult to navigate; Furthermore, `pathtype` doesn't solve the most fundamental problem with the `FilePath` type currently in `base`: its `String` representation. The only valid representation for cross-platform file paths is `ByteString`, because Windows paths can contain unpaired UTF-16 surrogates. Upcoming revisions of the library are switching it to a `ShortByteString` representation, which is the Right Thing. I think the lesson learned here is that "parse, don't validate" is not super practical when the entire world has built around validation of file paths rather than parsing them. Indeed, true validation of file paths would entail IO everywhere, as we'd want to check for the existence and validity of a file path.
19a613f
to
4df88eb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm in favour modulo CI (obvs) 👍🏻
vs.
welp @patrickt: I have no explanation for this. I don't think I broke it with my merge conflict fix, but I can't rule it out. Thoughts? (note that while these use the ruby fixtures, they are actually |
Running the pre-merge commit locally with 8.10.7, I don't see that error, but I do see 31 errors due to uncaught |
Same 31 failures with the |
@robrix Yeah I have absolutely no idea what this could be. Do we need to regenerate the fixtures or something? |
And why is it comparing one |
Let's see how far we get fixing the |
pathtype
was not a great success for us:readProjectFromPaths
;error
to check string literals, which is... fine, I guess, but doesn't help much in actuality;FilePath
was not an option, as libraries likedirectory-tree
andGlob
require it;Furthermore,
pathtype
doesn't solve the most fundamental problem with theFilePath
type currently inbase
: itsString
representation. The only validrepresentation for cross-platform file paths is
ByteString
, because Windowspaths can contain unpaired UTF-16 surrogates. Upcoming revisions of the library
are switching it to a
ShortByteString
representation, which is the Right Thing.I think the lesson learned here is that "parse, don't validate" is not super
practical when the entire world has built around validation of file paths rather
than parsing them. Indeed, true validation of file paths would entail IO
everywhere, as we'd want to check for the existence and validity of a file path.