Do not use isfile for URLs in checkpath_load #321
Conversation
|
@timholy Hm, I don't fully understand what is going on here. The test errors seem to indicate that this package is now somehow integrated with FilePaths.jl? I don't really know how to continue here, but I do know that 1.6 broke things on this aspect of things :) Any ideas how we could resolve this? |
| @@ -164,7 +164,7 @@ end | |||
| function checkpath_load(file) | |||
| file === nothing && return nothing # likely stream io | |||
| isa(file, IO) && return nothing | |||
| !isfile(file) && throw(ArgumentError("No file exists at given path: $file")) | |||
| startswith(file, "http://") || startswith(file, "https://") || !isfile(file) && throw(ArgumentError("No file exists at given path: $file")) | |||
There was a problem hiding this comment.
How about using a new helper function?
is_url(path::AbstractString) = startswith(path, "http://") || startswith(path, "https://")
is_url(path) = false| @@ -164,7 +164,7 @@ end | |||
| function checkpath_load(file) | |||
| file === nothing && return nothing # likely stream io | |||
| isa(file, IO) && return nothing | |||
| !isfile(file) && throw(ArgumentError("No file exists at given path: $file")) | |||
| startswith(file, "http://") || startswith(file, "https://") || !isfile(file) && throw(ArgumentError("No file exists at given path: $file")) | |||
There was a problem hiding this comment.
The logic seems a little bit hard to understand. If I understand it correctly, is the following an equivalent version?
| startswith(file, "http://") || startswith(file, "https://") || !isfile(file) && throw(ArgumentError("No file exists at given path: $file")) | |
| startswith(file, "http://") || startswith(file, "https://") || isfile(file) || throw(ArgumentError("No file exists at given path: $file")) |
|
Tests are needed to avoid #320 |
|
@timholy Bump :) The 1.6 update here broke functionality in CSVFiles.jl that had been working for years, would be good if we could somehow fix that, but I'm stuck, I don't understand what changes were introduced in FileIO.jl that caused this in the first place... |
Fixes #320. Not sure this is the best solution, but it would still be great if we could merge this quickly and tag a new release, so that the functionality in CSVFiles.jl is no longer broken.