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

Remove uses of pathtype library. #677

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

patrickt
Copy link
Contributor

@patrickt patrickt commented Apr 14, 2022

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.

@patrickt patrickt requested a review from a team as a code owner April 14, 2022 17:15
@patrickt patrickt force-pushed the the-pathtype-to-ruin branch 2 times, most recently from 5537ca9 to 19a613f Compare April 14, 2022 18:51
`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.
robrix
robrix previously approved these changes Apr 18, 2022
Copy link
Contributor

@robrix robrix left a 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) 👍🏻

@robrix
Copy link
Contributor

robrix commented Apr 21, 2022

1094

< {"files":[{"path":"semantic/test/fixtures/ruby/corpus/method-declaration.A.rb","language":"Ruby","symbols":[{"symbol":"foo","kind":"Method","line":"def foo","span":{"start":{"line":1,"column":5},"end":{"line":1,"column":8}},"nodeType":"DEFINITION","syntaxType":"METHOD","utf16CodeUnitSpan":{"start":{"column":4},"end":{"column":7}},"byteRange":{"start":4,"end":7}}]}]}

vs.

1096

> {"files":[{"path":"semantic/test/fixtures/ruby/corpus/and-or.A.rb","language":"Ruby","symbols":[{"symbol":"foo","kind":"Call","line":"foo and bar","span":{"start":{"line":1,"column":1},"end":{"line":1,"column":4}},"nodeType":"REFERENCE","syntaxType":"CALL","utf16CodeUnitSpan":{"start":{},"end":{"column":3}},"byteRange":{"end":3}},{"symbol":"bar","kind":"Call","line":"foo and bar","span":{"start":{"line":1,"column":9},"end":{"line":1,"column":12}},"nodeType":"REFERENCE","syntaxType":"CALL","utf16CodeUnitSpan":{"start":{"column":8},"end":{"column":11}},"byteRange":{"start":8,"end":11}}]}]}

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 semantic's tests, and not semantic-ruby's)

@robrix
Copy link
Contributor

robrix commented Apr 21, 2022

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 NoSuchFile exceptions.

@robrix
Copy link
Contributor

robrix commented Apr 21, 2022

Same 31 failures with the master-merged commit with 9.2.1.

@patrickt
Copy link
Contributor Author

@robrix Yeah I have absolutely no idea what this could be. Do we need to regenerate the fixtures or something?

@patrickt
Copy link
Contributor Author

And why is it comparing one method-declaration test and one and-or test? That doesn’t seem right. Is there some ordering nonsense being thrown in here?

@robrix
Copy link
Contributor

robrix commented Apr 21, 2022

Let's see how far we get fixing the NoSuchFile exceptions; maybe that error was spurious?

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

Successfully merging this pull request may close these issues.

None yet

2 participants