Merge pull request #82 from rofinn/rf/parsing

Replace `ispathtype` and string constructor with `tryparse`

Author: rofinn

src/FilePathsBase.jl

@@ -71,8 +71,8 @@ export isexecutable
 const PATH_TYPES = DataType[]
 
 function __init__()
-    register(PosixPath)
-    register(WindowsPath)
+    # Register the default fallback path type based on the os.
+    register(Sys.iswindows() ? WindowsPath : PosixPath)
 end
 
 """
@@ -88,8 +88,7 @@ Defines an abstract filesystem path.
 - `separator::String` - path separator (defaults to "/")
 
 # Required Methods
-- `T(str::String)` - A string constructor
-- `FilePathsBase.ispathtype(::Type{T}, x::AbstractString) = true`
+- `tryparse(::Type{T}, str::String)` - For parsing string representations of your path
 - `read(path::T)`
 - `write(path::T, data)`
 - `exists(path::T` - whether the path exists

src/path.jl

@@ -17,23 +17,7 @@ NOTE: `Path(::AbstractString)` can also work for custom paths if
 function Path end
 
 Path(fp::AbstractPath) = fp
-
-# May want to support using the registry for other constructors as well
-function Path(str::AbstractString; debug=false)
-    types = filter(t -> ispathtype(t, str), PATH_TYPES)
-
-    if length(types) > 1
-        @debug(
-            string(
-                "Found multiple path types that match the string specified ($types). ",
-                "Please use a specific constructor if $(first(types)) is not the correct type."
-            )
-        )
-    end
-
-    return first(types)(str)
-end
-
+Path(str::AbstractString) = parse(AbstractPath, str)
 function Path(fp::T, segments::Tuple{Vararg{String}}) where T <: AbstractPath
     T((s === :segments ? segments : getfield(fp, s) for s in fieldnames(T))...)
 end
@@ -78,8 +62,49 @@ function Base.show(io::IO, fp::AbstractPath)
     get(io, :compact, false) ? print(io, fp) : print(io, "p\"$fp\"")
 end
 
-Base.parse(::Type{<:AbstractPath}, x::AbstractString) = Path(x)
-Base.convert(::Type{<:AbstractPath}, x::AbstractString) = Path(x)
+# Default string constructors for AbstractPath types should fall back to calling `parse`.
+(::Type{T})(str::AbstractString) where {T<:AbstractPath} = parse(T, str)
+
+function Base.parse(::Type{P}, str::AbstractString; kwargs...) where P<:AbstractPath
+    result = tryparse(P, str; kwargs...)
+    result === nothing && throw(ArgumentError("$str cannot be parsed as $P"))
+    return result
+end
+
+function Base.tryparse(::Type{AbstractPath}, str::AbstractString; debug=false)
+    result = nothing
+    types = Vector{eltype(PATH_TYPES)}()
+
+    for P in PATH_TYPES
+        r = tryparse(P, str)
+
+        # If we successfully parsed the path then save that result
+        # and break if we aren't in debug mode, otherwise record how many
+        if r !== nothing
+            # Only assign the result if it's currently `nothing`
+            result = result === nothing ? r : result
+
+            if debug
+                push!(types, P)
+            else
+                break
+            end
+        end
+    end
+
+    if length(types) > 1
+        @debug(
+            string(
+                "Found multiple path types that could parse the string specified ($types). ",
+                "Please use a specific `parse` method if $(first(types)) is not the correct type."
+            )
+        )
+    end
+
+    return result
+end
+
+Base.convert(T::Type{<:AbstractPath}, x::AbstractString) = parse(T, x)
 Base.convert(::Type{String}, x::AbstractPath) = string(x)
 Base.promote_rule(::Type{String}, ::Type{<:AbstractPath}) = String
 Base.isless(a::P, b::P) where P<:AbstractPath = isless(a.segments, b.segments)
@@ -221,7 +246,7 @@ p"foobar"
 ```
 """
 function Base.:(*)(a::T, b::Union{T, AbstractString, AbstractChar}...) where T <: AbstractPath
-    T(*(string(a), string.(b)...))
+    return parse(T, *(string(a), string.(b)...))
 end
 
 """
@@ -393,18 +418,18 @@ function absolute(fp::AbstractPath)
 end
 
 """
-    relative{T<:AbstractPath}(fp::T, start::T=T("."))
+    relative{T<:AbstractPath}(fp::T, start::T=cwd())
 
 Creates a relative path from either the current directory or an arbitrary start directory.
 """
-function relative(fp::T, start::T=T(".")) where {T <: AbstractPath}
+function relative(fp::T, start::T=cwd()) where {T<:AbstractPath}
     curdir = "."
     pardir = ".."
 
     p = absolute(fp).segments
     s = absolute(start).segments
 
-    p == s && return T(curdir)
+    p == s && return parse(T, curdir)
 
     i = 0
     while i < min(length(p), length(s))
@@ -431,7 +456,7 @@ function relative(fp::T, start::T=T(".")) where {T <: AbstractPath}
     else
         relpath_ = tuple(pathpart...)
     end
-    return isempty(relpath_) ? T(curdir) : Path(fp, relpath_)
+    return isempty(relpath_) ? parse(T, curdir) : Path(fp, relpath_)
 end
 
 """

src/posix.jl

@@ -12,21 +12,15 @@ end
 PosixPath() = PosixPath(tuple(), "")
 PosixPath(segments::Tuple; root="") = PosixPath(segments, root)
 
-function PosixPath(str::AbstractString)
+function Base.tryparse(::Type{PosixPath}, str::AbstractString)
     str = string(str)
-    root = ""
-
     isempty(str) && return PosixPath(tuple("."))
 
     tokenized = split(str, POSIX_PATH_SEPARATOR)
-    if isempty(tokenized[1])
-        root = POSIX_PATH_SEPARATOR
-    end
+    root = isempty(tokenized[1]) ? POSIX_PATH_SEPARATOR : ""
     return PosixPath(tuple(map(String, filter!(!isempty, tokenized))...), root)
 end
 
-ispathtype(::Type{PosixPath}, str::AbstractString) = Sys.isunix()
-
 function Base.expanduser(fp::PosixPath)::PosixPath
     p = fp.segments
 

src/test.jl

@@ -13,7 +13,6 @@ ps = PathSet(; symlink=true)
 # Select the subset of tests to run
 # Inspect TestPaths.TESTALL to see full list
 testsets = [
-    test_constructor,
     test_registration,
     test_show,
     test_parse,
@@ -67,7 +66,6 @@ module TestPaths
     export PathSet,
         TESTALL,
         test,
-        test_constructor,
         test_registration,
         test_show,
         test_parse,
@@ -180,17 +178,12 @@ module TestPaths
         end
     end
 
-    function test_constructor(ps::PathSet{P}) where P <: AbstractPath
-        @testset "Constructor" begin
-            str = string(ps.root)
-            @test P(str) == ps.root
-        end
-    end
+    # NOTE: Most paths should test their own constructors as necessary.
 
     function test_registration(ps::PathSet{P}) where P <: AbstractPath
         @testset "Path constructor" begin
             str = string(ps.root)
-            @test FilePathsBase.ispathtype(P, str)
+            @test tryparse(P, str) !== nothing
             @test Path(str) == ps.root
             @test p"foo/bar" == Path("foo/bar")
         end
@@ -211,6 +204,7 @@ module TestPaths
         @testset "parsing" begin
             str = string(ps.root)
             @test parse(P, str) == ps.root
+            @test tryparse(P, str) == ps.root
         end
     end
 
@@ -898,7 +892,6 @@ module TestPaths
     end
 
     TESTALL = [
-        test_constructor,
         test_registration,
         test_show,
         test_parse,

src/windows.jl

@@ -28,17 +28,19 @@ function WindowsPath(segments::Tuple; root="", drive="", separator="\\")
     return WindowsPath(segments, root, drive, separator)
 end
 
-function WindowsPath(str::AbstractString)
+function Base.tryparse(::Type{WindowsPath}, str::AbstractString)
     isempty(str) && WindowsPath(tuple("."), "", "")
 
     if startswith(str, "\\\\?\\")
-        error("The \\\\?\\ prefix is currently not supported.")
+        @debug("The \\\\?\\ prefix is currently not supported.")
+        return nothing
     end
 
     str = replace(str, POSIX_PATH_SEPARATOR => WIN_PATH_SEPARATOR)
 
     if startswith(str, "\\\\")
-        error("UNC paths are currently not supported.")
+        @debug("UNC paths are currently not supported.")
+        return nothing
     elseif startswith(str, "\\")
         tokenized = split(str, WIN_PATH_SEPARATOR)
 
@@ -72,8 +74,6 @@ function Base.:(==)(a::WindowsPath, b::WindowsPath)
         lowercase(a.drive) == lowercase(b.drive)
 end
 
-ispathtype(::Type{WindowsPath}, str::AbstractString) = Sys.iswindows()
-
 function Base.show(io::IO, fp::WindowsPath)
     print(io, "p\"")
     if isabsolute(fp)

test/runtests.jl

@@ -19,7 +19,6 @@ include("testpkg.jl")
 
         @testset "$(typeof(ps.root))" begin
             testsets = [
-                test_constructor,
                 test_registration,
                 test_show,
                 test_parse,

test/system.jl

@@ -2,7 +2,6 @@ ps = PathSet(; symlink=true)
 
 @testset "$(typeof(ps.root))" begin
     testsets = [
-        test_constructor,
         test_registration,
         test_show,
         test_parse,
@@ -75,6 +74,9 @@ ps = PathSet(; symlink=true)
 
             p = Path(reg)
 
+            # Test calling the Path tryparse method with debug mode.
+            p = tryparse(AbstractPath, reg; debug=true)
+
             @test p == p"../src/FilePathsBase.jl"
             @test string(p) == reg
             @test string(cwd()) == pwd()

test/testpkg.jl

@@ -17,27 +17,20 @@ end
 
 TestPath() = TestPath(tuple(), "", "test:", ";")
 
-function TestPath(str::AbstractString)
+function Base.tryparse(::Type{TestPath}, str::AbstractString)
+    startswith(str, "test:") || return nothing
     str = String(str)
-
-    @assert startswith(str, "test:")
     drive = "test:"
-    root = ""
     str = str[6:end]
 
-    if isempty(str)
-        return TestPath(tuple("."), "", drive)
-    end
+    isempty(str) && return TestPath(tuple("."), "", drive)
 
     tokenized = split(str, ";")
-    if isempty(tokenized[1])
-        root = ";"
-    end
+    root = isempty(tokenized[1]) ? ";" : ""
 
     return TestPath(tuple(map(String, filter!(!isempty, tokenized))...), root, drive, ";")
 end
 
-FilePathsBase.ispathtype(::Type{TestPath}, str::AbstractString) = startswith(str, "test:")
 function test2posix(fp::TestPath)
     return PosixPath(fp.segments, isempty(fp.root) ? "" : "/")
 end