Expand Windows support + fix all specs #447
Conversation
| path += ".git" unless path.ends_with?(".git") | ||
| path = Path[path] | ||
| if (anchor = path.anchor) | ||
| path = Path[path.drive.to_s.rchop(":"), path.relative_to(anchor)] |
There was a problem hiding this comment.
Maybe add a comment that this transforms C:\local\path.git to C\local\path.git so it can be made relative to cache_path?
I'm also wondering if Path should handle that for you. With POSIX paths this is already working but Windows needs extra handling. Perhaps Path.windows("a").join("B:\\c") should eq Path.windows("a\\B\\c")? AFAIK you can't have colons in a windows path component except for the drive name.
There was a problem hiding this comment.
Comment done.
One thing I'm really sure of is that no, join should never consider putting the anchor in the middle of the path. It's doing it at the moment, though, just that nobody (except me) had given it a second thought before seeing this edge case. Respectfully, join's behavior on Windows is more useful than POSIX because at least in almost all cases it will fail with a loud bang (unfortunately indirectly though).
The fact that this strange operation is explicitly chosen here doesn't mean anything for general usefulness of this. It should stay explicit.
There was a problem hiding this comment.
Yeah, but maybe a method to transform an anchored path into a safe-to-use relative path could be a useful tool.
| @@ -357,6 +362,10 @@ module Shards | |||
|
|
|||
| # Parses a URI string, with additional support for ssh+git URI schemes. | |||
| private def parse_uri(raw_uri) | |||
| if (path = raw_uri.lchop?("file://")) | |||
There was a problem hiding this comment.
Would be nice to have a comment on this as well.
Also what about file:\\\\ URIs?
There was a problem hiding this comment.
Comment done.
I don't know, what about them? Backslashes are not valid here, and if there are 4 slashes, then I guess 2 will remain, and maybe Windows will still do something useful with them?
https://en.wikipedia.org/wiki/File_URI_scheme#How_many_slashes.3F
There was a problem hiding this comment.
Sry, I wrote the backslashes escaped. I just meant file:\\ followed by an anchored path. This is at least somewhat common on windows, although not valid as a URI.
Still, maybe adding || (path = raw_uri.lchop?("file:\\\\")) wouldn't be a bad idea?
src/commands/prune.cr
Outdated
| @@ -1,19 +1,20 @@ | |||
| require "file_utils" | |||
| require "./command" | |||
| require "../helpers/files" | |||
There was a problem hiding this comment.
This file should be moved:
| require "../helpers/files" | |
| require "../helpers" |
There was a problem hiding this comment.
It doesn't really matter either way.
There was a problem hiding this comment.
IMO it does from the good practices standpoint - i.e. naming files according to their namespace path Foo::Bar::Baz -> foo/bar/baz.cr
There was a problem hiding this comment.
Yes, that does actually seem better. Will include this change. No idea
There was a problem hiding this comment.
@Sija In general, that's true. But for this use case not so much IMO. A helpers namespace is intended to contain a mixture of different things (eventually). Otherwise you would use a more specifc namespace. When there are lots of helpers you may not want to place them in a single file at ./helpers but separted by concerns in ./helpers/*. Currently, we don't need that. But maybe in the future. It can be adapter later when necessary, but it wouldn't hurt to leave it as it was.
The change from helpers/files to helper IMO didn't help to make the PR any better, yet it requires another round of reviews (quick ones, obviously) when it could have just been merged as it was. 🤷♂️
| @@ -21,7 +21,11 @@ class Shards::Info | |||
| Dir.mkdir_p(@install_path) | |||
|
|
|||
| unless File.exists?(info_path) | |||
| Dir[File.join @install_path, "*.sha1"].each { |p| File.delete(p) } | |||
| Dir.each_child(@install_path) do |name| | |||
There was a problem hiding this comment.
Why is this needed? I thought glob were already supported in windows.
There was a problem hiding this comment.
There was a problem hiding this comment.
Also it's a change for the better because now file paths with * aren't a hazard
There was a problem hiding this comment.
Ok, but the culprit for this change is File.join then, not Dir#[]. I'm ok with the change, but I wanted to know the reason for it.
There was a problem hiding this comment.
each_child is actually better 👍
In general: glob patterns should just be written as string interpolation, not use File.join (you could use Path.posix but what's the point?).
|
Thanks @oprypin |
No description provided.