fix: #29 #52
fix: #29 #52
Conversation
|
|
files/files.go
Outdated
| func Getter(source, dst string) error { | ||
|
|
||
| // Validate the URL first. | ||
| url, err := url.ParseRequestURI(source) |
There was a problem hiding this comment.
Why do we need to validate the URL first? Are we sure that url.ParseRequestURI will not return an error for all URLS/URI's supported by go-getter ?
There was a problem hiding this comment.
I think you are right. I added the URI parser primarily for branching the code to handle local files and URLs separately. But then I forgot I shifted it to ResolveFile() function. I'm pushing the change.
files/files.go
Outdated
| // | ||
| // This will validate both a file in the filesystem, | ||
| // as well as the URL source for a file. | ||
| if filepath.Ext(file) == "" { |
There was a problem hiding this comment.
Why does a file or URL need an extension?
$value == 1.00 is not a valid path or URL, yet would pass this test?
And likewise
/etc/hosts is a valid path ?
There was a problem hiding this comment.
Aah, I only added the extension check to return an error if its a directory instead of a file. I totally forgot about files like /etc/hosts with no extensions. Let me improve this.
files/files.go
Outdated
| // Ensure the scheme is HTTP or HTTPS. | ||
| if err == nil && | ||
| url.Scheme != "" && | ||
| (strings.ToLower(url.Scheme) == "http" || |
There was a problem hiding this comment.
What about the other schemes supported by go-getter ?
There was a problem hiding this comment.
I only added the scheme check because FS files came with file:// scheme. And I wanted to exclude them. Because the url.ParseRequestURI(source) method is parsing even the URLs without a host as valid ones. So, I needed a quick way out.
There was a problem hiding this comment.
Oh, wait! I didn't know that go-getter also supports local files. I just checked their docs. I thought it only supported remote files over networks 🤦
This calls for changing the entire function code now.
Apologies for this mishap! I'll get this done.
There was a problem hiding this comment.
@moshloop, should I add an extra argument for the destination to where we have to resolve the file? The issue description doesn't mention it, that's why I am asking. In absence of a valid destination, I'll initialize a temporary directory to resolve the files over there.
There was a problem hiding this comment.
Pushed new changes. Supports all go-getter schemes now 👍 Including git://. Added a new test for it.
|
@mrinalwahal can you check the test failures? We can also drop support for 1.16 |
The test errors are related to Let me know if anything more is required. |
Since you updated the go version in go.mod to 1.18 we won't have the go1.17 working with test... But we would need to support it so I suggest support from 1.17 and then fix the go.mod |
Hey @Kaitou786, I've also tried adding support from both 1.16 and 1.17 onward. But dependency management is breaking. Refreshed the Also, I'd suggest from 1.16 onward because you seem to be running tests for it. And the error is always coming from the same package i.e. |
So would need to make pin to 1.16 on the go.mod?
you can try updating the GH setup/go action... I am able to compile your code locally with go1.17 |
|
So the checks seem to have worked with support only from Go 1.18 onward. But let me experiment once again with Go 1.17. If it doesn't work, we can switch to 1.18 again. |
|
Nope! Tried adding support for Go 1.17 and package So, I'm sticking with support from Go 1.18 onward only. |
Opening this PR as submission of my coding assignment.
This PR merges the following changes:
ResolveFile()andResolveFiles()functions.ResolveFile()with a correct filename as input, a directory and a URL.NOTE:
ResolveFiles()should ideally not exist. I'd say you should write a range to resolve multiple files whenever and wherever required. Its response is also not apt. There are better ways to return errors if any of those files can't be resolved. But I've kept the response strictly identical to the issue description.