Loosen filename type constraints #259
Conversation
Maintains the same public API (e.g., type constraints on methods remains the same), but allows FilePaths.jl to extend the public methods to work with `AbstractPath`s. This seemed preferable to depending directly on FilePathsBase or loosening the type constraints on all methods.
Codecov Report
@@ Coverage Diff @@
## master #259 +/- ##
==========================================
- Coverage 76.08% 76.02% -0.06%
==========================================
Files 8 8
Lines 414 413 -1
==========================================
- Hits 315 314 -1
Misses 99 99
Continue to review full report at Codecov.
|
rofinn
changed the title
rofinn
changed the title
|
I'll just mention this PR is easier to review with: https://github.com/JuliaIO/FileIO.jl/pull/259/files?w=1 |
|
Coverage says types.jl#31 isn't being hit anymore, but it should still be getting called by loadsave.jl#128 :/ |
|
Okay, yep, that constructor is definitely being hit by the tests cause switching it to an error breaks the tests. |
|
Are there any other concerns or is this safe to merge and tag? |
|
I think the only thing left to do is a minor version bump |
|
I'll just mention I did "Squash and merge" and GitHub flashed an error message and then just did a merge. Oh well ¯\_(ツ)_/¯ |
|
For some reason this passes when run with Load PosixPath: Error During Test at /home/tim/.julia/dev/FileIO/test/loadsave.jl:36
Test threw exception
Expression: load(joinpath(fp, "file1.pbm")) == "PBMText"
MethodError: no method matching splitext(::PosixPath)
Closest candidates are:
splitext(::String) at path.jl:201
splitext(::AbstractString) at path.jl:528
Stacktrace:
[1] query(filename::PosixPath; checkfile::Bool)
@ FileIO ~/.julia/dev/FileIO/src/query.jl:74
[2] query(filename::PosixPath)
@ FileIO ~/.julia/dev/FileIO/src/query.jl:73
[3] load(::PosixPath; options::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
@ FileIO ~/.julia/dev/FileIO/src/loadsave.jl:137
[4] load(::PosixPath)
@ FileIO ~/.julia/dev/FileIO/src/loadsave.jl:137
[5] macro expansion
@ ~/.julia/dev/FileIO/test/loadsave.jl:36 [inlined]
[6] macro expansion
@ ~/src/julia-master/usr/share/julia/stdlib/v1.6/Test/src/Test.jl:1222 [inlined]
[7] top-level scope
@ ~/.julia/dev/FileIO/test/loadsave.jl:27Can EDIT: nevermind, it's a Pkg-hold-back issue (AWSTools and Bonsai are holding it back, the first indirectly via BugReporting). |
This is holding back FilePathsBase, which is causing JuliaIO/FileIO.jl#259 (comment). If this works, it would be good to get a new release.
After #259, the result of `filename(q)` is not inferrable. This uses "the kernel trick" to provide a single point where the result must be inferred, and an `invokelatest` to prevent the abstract signature from being inferred. It then precompiles the `::String` variant. Consequently, with respect to latency this should get us back to where we were before #259.
This is holding back FilePathsBase, which is causing JuliaIO/FileIO.jl#259 (comment). If this works, it would be good to get a new release.
Maintains the same public API (e.g., type constraints on methods remains the same),but allows FilePaths.jl to extend the public methods to work with
AbstractPaths.This seemed preferable to depending directly on FilePathsBase or loosening the type constraints on all methods.
Loosen the restriction that filenames must be strings which will allow any
AbstractPathto also work with FileIO. This approach doesn't require an explicit dependency, but I've added it as a test dependency to help ensure thatload,save,query, etc work with the baseAbstractPathtypes.Partially addresses #219 with support from extended methods in FilePaths.jl. Also, closes rofinn/FilePaths.jl#34.