add Aqua test & fix all ambiguities and unbound args #115
Conversation
|
nvm, I fixed it for 1.0 |
Codecov Report
@@ Coverage Diff @@
## master #115 +/- ##
==========================================
- Coverage 96.91% 95.10% -1.82%
==========================================
Files 12 12
Lines 714 736 +22
==========================================
+ Hits 692 700 +8
- Misses 22 36 +14
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thanks a lot for this! Looks overall good, but I have a minor comment about the Measurement{T}(<:AbstractChar) method.
I'd have been ok with dropping support for v1.5-, but it looks like you've already done the needed changes, so we may as well just keep them and think about dropping support for old versions next time.
src/Measurements.jl
Outdated
| function Measurement{T}(::S) where {T, S<:AbstractChar} | ||
| throw(ArgumentError("cannot convert `$S` to `Measurement{$T}`")) | ||
| end |
There was a problem hiding this comment.
I'm surprised this is needed at all. Or anyway AbstractChar looks oddly specific, why that type and not anything else? Perhaps S shouldn't have any constraint (or S<:Any, which is the same) and this would be the generic fallback for non Real types?
There was a problem hiding this comment.
If you comment out this function, then you will see the following
1 ambiguities found
Ambiguity #1
Measurements.Measurement{T}(x::S) where {T, S} in Measurements at /home/roger/code/julia/Measurements/src/Measurements.jl:63
(::Type{T})(x::AbstractChar) where T<:Union{AbstractChar, Number} in Base at char.jl:50
Possible fix, define
Measurements.Measurement{T}(::S) where {T, S<:AbstractChar}I think this is due to the fact you can convert a Char to a Number defined in Base. And because Base uses AbstractChar so we have to use AbstractChar, why S <: Any is the same?
There was a problem hiding this comment.
actually, should it work for converting things to Measurement? since the following works
Float64('a')it is probably strange that meanwhile
Measurement{Float64}('a')
doesn't work. I'm not sure this is useful anyway tho, so either is fine
There was a problem hiding this comment.
Oh, yes, I'm looking forward to character arithmetic with uncertainties: 'z' ± 'a'! 😁
|
OK I just made |
There was a problem hiding this comment.
Only missing thing is to add some tests for the new methods, code coverage decreased 🙂
| function Measurement{T}(::S) where {T, S} | ||
| throw(ArgumentError("cannot convert `$S` to `Measurement{$T}`")) | ||
| end |
There was a problem hiding this comment.
I think this method isn't tested?
|
hmm looks like JuliaLang/julia#25085 is breaking the ambiguities here on nightly |
|
maybe we can fix the ambiguity of nightly later? I'd like to wait for JuliaTesting/Aqua.jl#71 to get merged before working on this. The tests should cover those methods now I believe, but not sure why some of them are not on codecov |
|
I'm not incredibly happy about merging a PR with failing tests, especially since they've been added just in this PR 🙃 But I take you're going to fix them soon™️, so let's go |
|
Well @giordano technically the changes in nightly Julia happens after I created the PR 🤷♂️. I assume in the future with Aqua test it will at least make the package eval fail for new Julia PRs then people could fix these new ambiguities caused by upstream easier. |
No description provided.