Support TimeSpan construction from Dates.CompoundPeriod, Dates.Period

[hannahilea]

No description provided.

[ararslan]

CI is failing on 1.6 because the convert method used here was introduced in v1.8.0 (specifically v1.8.0-DEV.300 via JuliaLang/julia#40803). You can add a dependency on Compat with Compat = "3.38.0" as its Project.toml compatibility specification, which will make this method available on earlier Julia versions.

[hannahilea]

Okay, updated to fix failing 1.6 tests!

[ericphanson]

I wonder if we should just always convert? That is the default behavior when you don't have an inner constructor, for example;

julia> struct A
       n::Nanosecond
       end
julia> A(1)
ERROR: MethodError: Cannot `convert` an object of type Int64 to an object of type Nanosecond
Closest candidates are:
  convert(::Type{T}, ::Dates.CompoundPeriod) where T<:Period at ~/.julia/juliaup/julia-1.8.5+0.x64.linux.gnu/share/julia/stdlib/v1.8/Dates/src/periods.jl:363
  convert(::Type{Nanosecond}, ::Microsecond) at ~/.julia/juliaup/julia-1.8.5+0.x64.linux.gnu/share/julia/stdlib/v1.8/Dates/src/periods.jl:433
  convert(::Type{Nanosecond}, ::Millisecond) at ~/.julia/juliaup/julia-1.8.5+0.x64.linux.gnu/share/julia/stdlib/v1.8/Dates/src/periods.jl:433
  ...
Stacktrace:
 [1] A(n::Int64)
   @ Main ./REPL[7]:2
 [2] top-level scope
   @ REPL[11]:1

You can see in the stacktrace that it tries to call convert. But if you have an inner constructor, it calls that and doesn't have the default constructor w/ convert:

julia> struct B
       n::Nanosecond
       B(n::Nanosecond) = new(n)
       end

julia> B(1)
ERROR: MethodError: no method matching B(::Int64)
Closest candidates are:
  B(::Nanosecond) at REPL[9]:3
Stacktrace:
 [1] top-level scope
   @ REPL[10]:1

I believe we have the inner constructor to do the start == stop case, but I don't think we were aiming to get rid of the convert constructors. So I think just auto-converting when we don't hit the ::Nanosecond one might be the best approach:

Suggested change

	_to_ns = (t) -> isa(t, Dates.CompoundPeriod) ? convert(Nanosecond, t) : Nanosecond(t)
	_to_ns = convert(Nanosecond, t)

Note that convert(Nanosecond, Nanosecond(1)) works, so even if start::CompoundPeriod and stop::Nanosecond, the extra convert should be harmless.

[hannahilea]

we should! we weren't b/c I thought that Alex's "Compat" fix wasn't working, because I didn't implement it correctly :lol-cry:. BUT the most recent implementation does what you're suggesting, I think---take a look.

[hannahilea]

oh hang on! let me reread

[ericphanson]

ah oops, yeah, agree w/ the current implementation!

I was suggesting something different (always convert) but I missed that we already had

TimeSpan(start, stop) = TimeSpan(Nanosecond(start), Nanosecond(stop))

in the code base, in which case using the constructor (Nanosecond()) instead of convert makes sense as the fallback. Ok agreed with what we have!