Drop string subtyping #22

rofinn · 2019-01-27T19:44:49Z

Closes #15

codecov-io · 2019-01-27T19:55:25Z

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 · 2019-01-27T22:26:59Z

@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 · 2019-01-27T22:40:18Z

src/FilePathsBase.jl

-
 Base.convert(::Type{AbstractPath}, x::AbstractString) = Path(x)
-ispathtype(::Type{T}, x::AbstractString) where {T<:AbstractPath} = false
+Base.convert(::Type{String}, x::AbstractPath) = string(x)


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"

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.

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 · 2019-01-27T22:42:09Z

src/mode.jl


 Base.show(io::IO, mode::Mode) = print(io, string(mode))
-Base.String(mode::Mode) = string(mode)
+Base.string(mode::Mode) = String(mode)


One should not overload string, only overload print.

JuliaLang/julia#30757 (comment)

So these definitions kind of need to be flipped

Should I just overload show and print?

That is my understanding, yes.

oxinabox · 2019-01-27T22:45:31Z

src/path.jl


 """
    exists(path::AbstractPath) -> Bool



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?

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

…parse` methods for `Mode` and `<:AbstractPath`.

davidanthoff · 2019-01-28T19:03:45Z

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 :)

src/mode.jl

Co-Authored-By: rofinn <[email protected]>

rofinn · 2019-01-28T23:14:32Z

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

…show and print methods.

rofinn · 2019-01-29T16:19:26Z

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

Drop string subtyping.

f061620

rofinn mentioned this pull request Jan 27, 2019

What inplace operator should we use for path concatenation? #2

Closed

rofinn added 3 commits January 27, 2019 14:44

Cleaned up stringification code and add a few extra tests.

1993958

Enable appveyor coverage.

d0a5d26

Reverse windows path String -> string definition.

53ba6ce

oxinabox reviewed Jan 27, 2019

View reviewed changes

Cleaned up the Mode String/string/print/show code and added `…

007e189

…parse` methods for `Mode` and `<:AbstractPath`.

rofinn force-pushed the rf/not-string branch from ab26b17 to 007e189 Compare January 28, 2019 01:39

oxinabox reviewed Jan 28, 2019

View reviewed changes

src/mode.jl Outdated Show resolved Hide resolved

oxinabox reviewed Jan 28, 2019

View reviewed changes

src/mode.jl Outdated Show resolved Hide resolved

oxinabox and others added 2 commits January 28, 2019 17:10

Mode bitwise or assignment

7cdada9

Co-Authored-By: rofinn <[email protected]>

show using direct mode string interpolation

550399b

Co-Authored-By: rofinn <[email protected]>

rofinn added 2 commits January 28, 2019 22:37

Add support for open.

c2ae0a1

Drop String constructors and restrict string related output to a few …

bc802d5

…show and print methods.

rofinn merged commit dff9713 into master Jan 29, 2019

oxinabox mentioned this pull request Mar 26, 2019

Relax type constraints on filenames JuliaData/CSV.jl#394

Closed

Drop string subtyping #22

Drop string subtyping #22

Uh oh!

Conversation

rofinn commented Jan 27, 2019

Uh oh!

codecov-io commented Jan 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rofinn commented Jan 27, 2019

Uh oh!

oxinabox Jan 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rofinn Jan 28, 2019

Choose a reason for hiding this comment

Uh oh!

oxinabox Jan 28, 2019

Choose a reason for hiding this comment

Uh oh!

oxinabox Jan 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rofinn Jan 28, 2019

Choose a reason for hiding this comment

Uh oh!

oxinabox Jan 28, 2019

Choose a reason for hiding this comment

Uh oh!

oxinabox Jan 27, 2019

Choose a reason for hiding this comment

Uh oh!

rofinn Jan 28, 2019

Choose a reason for hiding this comment

Uh oh!

davidanthoff commented Jan 28, 2019

Uh oh!

Uh oh!

Uh oh!

rofinn commented Jan 28, 2019

Uh oh!

rofinn commented Jan 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Jan 27, 2019 •

edited

Loading

oxinabox Jan 27, 2019 •

edited

Loading

oxinabox Jan 27, 2019 •

edited

Loading