Skip to content

Conversation

@rofinn
Copy link
Owner

@rofinn rofinn commented Apr 15, 2020

Closes #79 and #78

@codecov
Copy link

codecov bot commented Apr 15, 2020

Codecov Report

Merging #82 into master will decrease coverage by 3.94%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #82      +/-   ##
==========================================
- Coverage   91.34%   87.40%   -3.95%     
==========================================
  Files          12       12              
  Lines         959     1032      +73     
==========================================
+ Hits          876      902      +26     
- Misses         83      130      +47     
Impacted Files Coverage Δ
src/windows.jl 83.33% <0.00%> (-7.58%) ⬇️
src/posix.jl 77.27% <50.00%> (-4.55%) ⬇️
src/path.jl 84.12% <95.23%> (-0.16%) ⬇️
src/FilePathsBase.jl 100.00% <100.00%> (ø)
src/test.jl 93.31% <100.00%> (-5.90%) ⬇️
src/libc.jl 58.33% <0.00%> (-9.41%) ⬇️
src/system.jl 85.27% <0.00%> (-5.57%) ⬇️
src/buffer.jl 87.50% <0.00%> (-2.83%) ⬇️
... and 4 more

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 63b3e66...e70e784. Read the comment docs.

rofinn and others added 2 commits April 16, 2020 13:40
Co-Authored-By: Lyndon White <oxinabox@ucc.asn.au>
@rofinn
Copy link
Owner Author

rofinn commented Apr 16, 2020

@oxinabox Does this seem like an appropriate use of tryparse/parse?

@oxinabox
Copy link
Contributor

My initial (incorrect) instinct is, "o, parse should be for thigns that are valid julia literals".
But that is just wrong.

It aligns with the following methods:

julia> methods(parse)
# 12 methods for generic function "parse":


[5] parse(::Type{LibGit2.GitCredential}, url::AbstractString) in LibGit2 at /usr/local/src/julia/julia-1.4/usr/share/julia/stdlib/v1.4/LibGit2/src/gitcredential.jl:73
[6] parse(::Type{LibGit2.GitCredentialHelper}, helper::AbstractString) in LibGit2 at /usr/local/src/julia/julia-1.4/usr/share/julia/stdlib/v1.4/LibGit2/src/gitcredential.jl:163

[7] parse(::Type{Sockets.IPv4}, str::AbstractString) in Sockets at /usr/local/src/julia/julia-1.4/usr/share/julia/stdlib/v1.4/Sockets/src/IPAddr.jl:176
[8] parse(::Type{Sockets.IPv6}, str::AbstractString) in Sockets at /usr/local/src/julia/julia-1.4/usr/share/julia/stdlib/v1.4/Sockets/src/IPAddr.jl:227
[9] parse(::Type{Sockets.IPAddr}, str::AbstractString) in Sockets at /usr/local/src/julia/julia-1.4/usr/share/julia/stdlib/v1.4/Sockets/src/IPAddr.jl:247

[10] parse(::Type{DateTime}, s::AbstractString, df::DateFormat{Symb... }) in Dates at /usr/local/src/julia/julia-1.4/usr/share/julia/stdlib/v1.4/Dates/src/parse.jl:200
[11] parse(::Type{T}, str::AbstractString) where T<:TimeType in Dates at /usr/local/src/julia/julia-1.4/usr/share/julia/stdlib/v1.4/Dates/src/parse.jl:281
[12] parse(::Type{T}, str::AbstractString, df::DateFormat) where T<:TimeType in Dates at /usr/local/src/julia/julia-1.4/usr/share/julia/stdlib/v1.4/Dates/src/parse.jl:281

The ones it doesn't are:

[1] parse(::Type{T}, c::AbstractChar; base) where T<:Integer in Base at parse.jl:41
[2] parse(::Type{T}, s::AbstractString; base) where T<:Integer in Base at parse.jl:238
[3] parse(::Type{T}, s::AbstractString; kwargs...) where T<:Real in Base at parse.jl:376
[4] parse(::Type{T}, s::AbstractString) where T<:Complex in Base at parse.jl:378

@rofinn
Copy link
Owner Author

rofinn commented Apr 16, 2020

Yeah, I was mostly thinking of Sockets and Dates libs when I chose to do this. Should I check with the core devs in the julia slack to confirm that this is how the function is intended to be used?

@oxinabox
Copy link
Contributor

Should I check with the core devs in the julia slack to confirm that this is how the function is intended to be used?

You can, but i think it is clear you do have it right

@rofinn rofinn merged commit 3812bc1 into master Apr 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use parse(T, str) over constructors

3 participants