Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add Aqua test & fix all ambiguities and unbound args #115

Merged
merged 10 commits into from
Mar 1, 2022
Merged

Conversation

Roger-luo
Copy link
Contributor

No description provided.

@Roger-luo
Copy link
Contributor Author

Roger-luo commented Feb 24, 2022

hmmm... I have to add a bunch of 1.0 specific fixes I'm wondering if it's easier to just drop 1.0 support and start supporting the new LTS 1.6 instead? @giordano

nvm, I fixed it for 1.0

@codecov-commenter
Copy link

codecov-commenter commented Feb 24, 2022

Codecov Report

Merging #115 (ecb8880) into master (c79947c) will decrease coverage by 1.81%.
The diff coverage is 36.00%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
src/comparisons-tests.jl 80.00% <0.00%> (-20.00%) ⬇️
src/math.jl 98.42% <14.28%> (-1.58%) ⬇️
src/Measurements.jl 72.72% <25.00%> (-16.17%) ⬇️
src/conversions.jl 100.00% <100.00%> (+10.52%) ⬆️

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 c79947c...ecb8880. Read the comment docs.

Copy link
Member

@giordano giordano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 72 to 74
function Measurement{T}(::S) where {T, S<:AbstractChar}
throw(ArgumentError("cannot convert `$S` to `Measurement{$T}`"))
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yes, I'm looking forward to character arithmetic with uncertainties: 'z' ± 'a'! 😁

@Roger-luo
Copy link
Contributor Author

OK I just made Measurement{Float64}('a') work, and let the general non Real case error

Copy link
Member

@giordano giordano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only missing thing is to add some tests for the new methods, code coverage decreased 🙂

Comment on lines +72 to +74
function Measurement{T}(::S) where {T, S}
throw(ArgumentError("cannot convert `$S` to `Measurement{$T}`"))
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this method isn't tested?

@Roger-luo
Copy link
Contributor Author

Roger-luo commented Mar 1, 2022

hmm looks like JuliaLang/julia#25085 is breaking the ambiguities here on nightly

@Roger-luo
Copy link
Contributor Author

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

@giordano
Copy link
Member

giordano commented Mar 1, 2022

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

@giordano giordano merged commit d2edc82 into master Mar 1, 2022
@giordano giordano deleted the roger/aqua branch March 1, 2022 20:28
@Roger-luo
Copy link
Contributor Author

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.

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.

3 participants