Explicitly extend Base functions (Base.foo(...) = ...) instead of imports.#52
Explicitly extend Base functions (Base.foo(...) = ...) instead of imports.#52
Conversation
…orts. Style concensus seems to be that extending via `imports` should be discouraged since it can lead to both accidentally extending functions, and accidentally _not_ extending the intended functions. This is a style-only change; there should be no substantive change at all.
|
@TotalVerb I opened this as a follow-up to the last PR (#51), since i accidentally messed up at first by creating a new, local |
NHDaly
force-pushed
the
nhd-explicit-base-extending
branch
from
fe63195 to
317c82b
Compare
Codecov Report
@@ Coverage Diff @@
## master #52 +/- ##
==========================================
+ Coverage 97.76% 97.77% +0.01%
==========================================
Files 1 1
Lines 179 180 +1
==========================================
+ Hits 175 176 +1
Misses 4 4
Continue to review full report at Codecov.
|
src/FixedPointDecimals.jl
Outdated
| """ | ||
| Base.@pure function max_exp10(::Type{T}) where {T <: Integer} | ||
| function max_exp10(::Type{T}) where {T <: Integer} | ||
| # This function is marked as `Base.@pure`. Even though it does call some generic |
There was a problem hiding this comment.
Oops. I don't think i actually meant for this change to be part of this PR... BUT i guess it doesn't hurt. It is definitely an improvement, so i'll just carry it along. Sorry about that; thanks for the careful review.
Co-Authored-By: Curtis Vogt <curtis.vogt@gmail.com>
Use `using Base: @pure` instead of `Base.@pure` everywhere. Applies @omus's PR review suggestion.
NHDaly
left a comment
NHDaly
left a comment
There was a problem hiding this comment.
Thanks both for the review. :) Haha sorry for my long delays between work here and thanks again :)
I'll go ahead and merge now.
src/FixedPointDecimals.jl
Outdated
| """ | ||
| Base.@pure function max_exp10(::Type{T}) where {T <: Integer} | ||
| function max_exp10(::Type{T}) where {T <: Integer} | ||
| # This function is marked as `Base.@pure`. Even though it does call some generic |
There was a problem hiding this comment.
Oops. I don't think i actually meant for this change to be part of this PR... BUT i guess it doesn't hurt. It is definitely an improvement, so i'll just carry it along. Sorry about that; thanks for the careful review.
src/FixedPointDecimals.jl
Outdated
| """ | ||
| Base.@pure function max_exp10(::Type{T}) where {T <: Integer} | ||
| function max_exp10(::Type{T}) where {T <: Integer} | ||
| # This function is marked as `Base.@pure`. Even though it does call some generic |
5 participants
Explicitly extend Base functions (via
Base.foo(...) = ...) instead of importing the names to be extended.Switches to
using Base: ...for non-extending imports (only fordecompose).Style consensus seems to be that extending via
importsshould bediscouraged since it can lead to both accidentally extending functions,
and accidentally not extending the intended functions.
This is a style-only change; there should be no substantive change at all.