oxinabox / DataDepsGenerators.jl

Utility for developers to help define DataDeps registration blocks, for reusing existing Data with DataDeps.jl
Other
18 stars 6 forks source link

Provision to add checksum in Register Blocks #18

Closed SebastinSanty closed 6 years ago

codecov-io commented 6 years ago

Codecov Report

Merging #18 into master will increase coverage by 0.74%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #18      +/-   ##
==========================================
+ Coverage   95.04%   95.79%   +0.74%     
==========================================
  Files           6        6              
  Lines         101      119      +18     
==========================================
+ Hits           96      114      +18     
  Misses          5        5
Impacted Files Coverage Δ
src/UCI.jl 100% <ø> (ø) :arrow_up:
src/DataDryad.jl 100% <100%> (ø) :arrow_up:
src/DataDepsGenerators.jl 100% <100%> (ø) :arrow_up:

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 da320e1...59008c3. Read the comment docs.

SebastinSanty commented 6 years ago

Thanks for the review! That testset gives me an idea of the checksums should be structured.

oxinabox commented 6 years ago

The kind of solution I was hoping to have for format_checksums was:

format_checksums(csum::AbstractString) = "\"$csum\""

function format_checksums(csum::Tuple{Symbol, <:AbstractString})
    func = string(csum[1])
    hashstring = format_checksums(csum[2])
    "($func, $hashstring)"
end

function format_checksums(csums::Vector)
    fcsums = join(format_checksums.(csums), ", ")
    "[$fcsums]"
end

Which is broadly similar to what you have. The key idea that is missing is having format_checksums call itself, and letting it sort things via dispatch on type. Rather than repeating every combination of options.

Also I'ld suggest being careful with how much you use replace to solve problems. This package code I wrote is maybe is setting a bit of a bad example. Direct string-manipulation is a bit of a hack.

SebastinSanty commented 6 years ago

I am not sure how I can convert :md5 to md5 without using replace(). Am I missing something?

oxinabox commented 6 years ago

yep, the string function. (Not to be confused with the String constructor)

julia> x = :md5
:md5

julia> typeof(x)                                                                                                                        Symbol

julia> string(x)
"md5"

julia> string(:anything)
"anything"
oxinabox commented 6 years ago

Broadly speaking looking good now: lets see what CI says

For your education I suggest reading and understanding https://docs.julialang.org/en/stable/manual/functions/#man-vectorized-1 I where possible, I prefer to write foo.(xs) over map(foo, xs) or [foo(x) for x in xs] though it is mostly a stylistic choice.

Also can you explain to me (and perhaps yourself) why you wrote: format_checksums(csum::Tuple{T,<:AbstractString}) where T<:Symbol

and not format_checksums(csum::Tuple{Symbol,<:AbstractString})

It isn't wrong as such. But it is worth understanding the difference between Foo(x::Vector{Bar}) vs foo(x::Vector{T}) where T<: Bar (also written foo(x::Vector{<:Bar}) When they are the same and when they are different

SebastinSanty commented 6 years ago

From what I understand, Foo(x::Vector{Bar}) will strictly work for Bar type arguments (in the Vector) whereas Foo(x::Vector{<:Bar}) will work for arguments of any type (in the Vector) which comes under the Bar hierarchy.

oxinabox commented 6 years ago

Cool, you've got that basically right. So the next trick is that we have abstract types (like AbstractString, Real, AbstractArray etc), and concrete types (like String, Float32, Array). Only abstract types can have subtypes.

Symbol is a concrete type, so no subtypes. Thus the only thing matched by T<:Symbol is Symbol. So rather than ever saying Foo(::Vector{<:Symbol}) one can just say Foo(::Vector{Symbol}).

SebastinSanty commented 6 years ago

Should I be changing it?

oxinabox commented 6 years ago

It doesn't really matter to me if you change it in this PR or just catch it later, or leave it

oxinabox commented 6 years ago

Merged :tada: