-
Notifications
You must be signed in to change notification settings - Fork 113
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
Support mixed argument types to div
and friends
#317
Conversation
Codecov Report
@@ Coverage Diff @@
## master #317 +/- ##
==========================================
- Coverage 81.39% 78.74% -2.66%
==========================================
Files 15 15
Lines 1129 1129
==========================================
- Hits 919 889 -30
- Misses 210 240 +30
Continue to review full report at Codecov.
|
I've also added a version bump for a patch release. Hope that's okay |
I just had a look at
You might want to have this supported as well. |
Good, point. The other thing is that in more recent versions of Julia we are only suppose to overload |
src/quantities.jl
Outdated
end | ||
|
||
function div(x::AbstractQuantity, y::Number, r::RoundingMode=RoundToZero) | ||
Quantity(div(ustrip(x), y, r), unit(x) / unit(y)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unit(y)
is a free unit so it is just unit(x)
isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're correct. I was just copying the /
method above. I'll fix it.
Not sure why this worked for me on 1.4 but is broken on everything earlier. I'll look at it more later today. |
The feature I meant with the rounding mode is supported since 1.4 as far as I can see. |
Not sure how to deal with it maybe @giordano can help. You probably want to support Julia 1.0. |
src/quantities.jl
Outdated
function div(x::AbstractQuantity, y::AbstractQuantity, r::RoundingMode=RoundToZero) | ||
z = uconvert(unit(y), x) # TODO: use promote? | ||
div(z.val,y.val, r) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd do
function div(x::AbstractQuantity, y::AbstractQuantity, r::RoundingMode=RoundToZero) | |
z = uconvert(unit(y), x) # TODO: use promote? | |
div(z.val,y.val, r) | |
end | |
function div(x::AbstractQuantity, y::AbstractQuantity, r...) | |
z = uconvert(unit(y), x) # TODO: use promote? | |
div(z.val,y.val, r...) | |
end |
so you don't even have to care to forcibly set a default value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds better than my idea.
Oh, then I just need to put the div code with the extra parameter inside a `if VERSION > v"1.3"`` block. |
This simply makes it so that when something like
div(9m, 3)
it produces3m
. Currently this errors instead.