Drop string subtyping

[rofinn]

Closes #15

[codecov-io]

Codecov Report

Merging #22 into master will increase coverage by 9.73%.
The diff coverage is 85.48%.

@@            Coverage Diff            @@
##           master     #22      +/-   ##
=========================================
+ Coverage   76.86%   86.6%   +9.73%     
=========================================
  Files           8       8              
  Lines         428     448      +20     
=========================================
+ Hits          329     388      +59     
+ Misses         99      60      -39

Impacted Files	Coverage Δ
src/windows.jl	81.81% <ø> (+59.59%)	⬆️
src/posix.jl	64% <100%> (+1.03%)	⬆️
src/FilePathsBase.jl	54.54% <33.33%> (+0.69%)	⬆️
src/path.jl	91.1% <90.9%> (+3.54%)	⬆️
src/mode.jl	92.13% <90.9%> (-2.07%)	⬇️
src/libc.jl	66.66% <0%> (+20.23%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 47de002...bc802d5. Read the comment docs.

[rofinn]

@davidanthoff Would you mind looking this over when you get a chance? These changes are likely breaking and you're the next most familiar person with the code base.

[oxinabox]

This should be String(x) not string(x).

Both semantically, and because string(x) gives the wrong result.
(at least with the previous version, you have changed this now maybe?)

julia> pp=Path(pwd())
p"//Users/oxinabox/JuliaEnvs/MagneticReadHead"

julia> string(pp)
p"//Users/oxinabox/JuliaEnvs/MagneticReadHead"

julia> String(pp)
"/Users/oxinabox/JuliaEnvs/MagneticReadHead"

[rofinn]

That's changing with the show and print changes.

julia> using FilePathsBase
[ Info: Recompiling stale cache file /Users/rory/.playground/share/filepaths/depot/compiled/v1.0/FilePathsBase/Efv1v.ji for FilePathsBase [48062228-2e41-5def-b9a4-89aafe57970f]

julia> p = cwd()
p"//Users/rory/repos/FilePathsBase.jl"

julia> string(p)
"/Users/rory/repos/FilePathsBase.jl"

julia> String(p)
"/Users/rory/repos/FilePathsBase.jl"

While this achieves what I'd like, I'm not sure it's the correct way to handle that.

[oxinabox]

String(p) feels more semantic to me.
It feels like a shorthand for convert(String, p)

Where as string(p) to me reads like "Give me some thing I could show a user".
Since it is tied to just being print.

Further, I think some AbstractPaths may want to overload their print in other ways.
Since print is pretty freefrom in what you do.
Like an S3Path might want to actually display the URL rather than (just) the path.

[oxinabox]

One should not overload string, only overload print.

JuliaLang/julia#30757 (comment)

So these definitions kind of need to be flipped

[rofinn]

Should I just overload show and print?

[oxinabox]

That is my understanding, yes.

[oxinabox]

All uses of string here should become String.
Since we are not wanting to work with a readable representation of the object (which si what string should do)
but with a String version of the object, which is what String should do.
(at least in analogy to the String(::AbstractString) method docs)
Really it is more of a convert but that is too verbose, no?

[rofinn]

I'm thinking it might be better to just overload show and print. I suppose an argument for using String is that it avoids an extra copy.

julia> @benchmark string($p)
BenchmarkTools.Trial:
  memory estimate:  1.53 KiB
  allocs estimate:  36
  --------------
  minimum time:     2.006 μs (0.00% GC)
  median time:      2.104 μs (0.00% GC)
  mean time:        2.711 μs (18.11% GC)
  maximum time:     3.403 ms (99.90% GC)
  --------------
  samples:          10000
  evals/sample:     10

julia> @benchmark String($p)
BenchmarkTools.Trial:
  memory estimate:  1.31 KiB
  allocs estimate:  32
  --------------
  minimum time:     1.906 μs (0.00% GC)
  median time:      1.960 μs (0.00% GC)
  mean time:        2.543 μs (18.13% GC)
  maximum time:     3.077 ms (99.87% GC)
  --------------
  samples:          10000
  evals/sample:     10

[davidanthoff]

I'm very much in favor of this change!

I took a very cursory look over the PR, and it all looks good to me. I won't have the time for a thorough review anytime soon, so I'd suggest to just go ahead and merge it :)

[rofinn]

One thing that I just thought of is that this will break open with AbstractPaths.

julia> open(p"README") do io
           println(read(io, String))
           end
ERROR: MethodError: no method matching open(::PosixPath)
Closest candidates are:
  open(::AbstractString; read, write, create, truncate, append) at iostream.jl:275
  open(::AbstractString, ::AbstractString) at iostream.jl:339
  open(::Function, ::Base.AbstractCmd, ::Any...) at process.jl:615

[rofinn]

Alright, I ran tests for a few dependent packages (e.g., VegaDatasets.jl) and this doesn't seem to break everything.