Re-enable Windows CI#324
Conversation
bbb21b0 to
032008f
Compare
032008f to
ead5eab
Compare
dd0f411 to
61ae2ef
Compare
ead5eab to
9ab0070
Compare
|
|
||
| uri.path | ||
| path = uri.path | ||
| path.delete_prefix!("/") if Gem.win_platform? |
There was a problem hiding this comment.
This is terrible, but temporary until we move the Ruby LSP's custom URI implementation into Saturn.
The reason we need this is because Windows paths are prefixed with an artificial forward slash to turn them into valid URIs. For example:
C:/projects/foo.rb
# Simply prefixing this with the scheme file:// will result in an invalid URI
# You need an extra forward slash (which has to be accounted for when translating
# back into a file path)
file:///C:/projects/foo.rb
| def uri_to(relative_path) | ||
| path = absolute_path_to(relative_path) | ||
| path.prepend("/") if Gem.win_platform? | ||
| URI::File.build(path: path).to_s |
There was a problem hiding this comment.
Will also be replaced by our custom URI abstraction once we bring that over.
| let baz = Path::new("bar").join("baz.rb"); | ||
| let qux = Path::new("bar").join("qux.rb"); | ||
| let bar = Path::new("foo").join("bar.rb"); |
There was a problem hiding this comment.
Can't we move this logic into Context.touch?
There was a problem hiding this comment.
I tried doing it in this commit, but it honestly makes the tests odd.
When these are running on Windows we need to assert that the result is using backslashes, which we can easily get if we simply do path.to_str().
If we try to continue writing tests with string literals that use forward slashes, then we need to somehow account for the fact that the result will contain forward slashes in Unix and backslashes on Windows - but then at that point, I feel like just dealing with paths is more straight forward.
| class Foo | ||
| # Method comment | ||
| def foo; end | ||
| def bar; end |
There was a problem hiding this comment.
Why do we need to change this?
There was a problem hiding this comment.
I wanted to extract the comments into a variable to make the test a bit more readable, but then both variables would be called foo_comments because both definitions are called foo.
Unless we're testing something related to indexing getting potentially confused due to similar names for different definitions, I'd vote to always use a different name to avoid any confusion.
There was a problem hiding this comment.
I think in this particular case we can also use class_comments and method_comments without changing the test itself. But the current version is fine to me too.
9ab0070 to
5819c80
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Regarding the caution tape bot, we are using the custom runners for Ubuntu and Windows, but we don't have custom runners for MacOS. |
5819c80 to
1edf741
Compare
st0012
left a comment
st0012
left a comment
There was a problem hiding this comment.
There are several pieces of code you mentioned that can be removed/simplified once we port the URI abstraction from ruby-lsp. Can you add todo comments on them, like TODO: revisit this once URI abstraction is ported from ruby-lsp? This will make sure others can easily pick them up too.
1edf741 to
fa4c7ac
Compare
|
Added the TODO comments 👍 |
3 participants
Closes #68
It seems that ruby/prism#3594 did fix the Windows build 🎉. This PR re-enables it so that we don't accidentally add Windows incompatible code.
Most of the fixes are related to assuming forward slashes in file paths or not accounting for the carriage return character where it may happen.