Add DataAPI as a dependency to share describe with other packages #496
Conversation
| @@ -648,12 +648,12 @@ Pretty-print the summary statistics provided by [`summarystats`](@ref): | |||
| the mean, minimum, 25th percentile, median, 75th percentile, and | |||
| maximum. | |||
| """ | |||
| describe(a::AbstractArray) = describe(stdout, a) | |||
| function describe(io::IO, a::AbstractArray{T}) where T<:Union{Real,Missing} | |||
| DataAPI.describe(x) = describe(stdout, x) | |||
There was a problem hiding this comment.
We should reexport describe to avoid breaking any packages which may be overloading it from StatsBase.
|
Alright, now that DataAPI.jl is registered, I've updated the PR here. |
|
Errors are just 0.7 staleness; should we remove that from testing? |
| @@ -648,12 +648,12 @@ Pretty-print the summary statistics provided by [`summarystats`](@ref): | |||
| the mean, minimum, 25th percentile, median, 75th percentile, and | |||
| maximum. | |||
| """ | |||
| describe(a::AbstractArray) = describe(stdout, a) | |||
| function describe(io::IO, a::AbstractArray{T}) where T<:Union{Real,Missing} | |||
| DataAPI.describe(x) = describe(stdout, x) | |||
There was a problem hiding this comment.
This catch-all definition is a type-piracy kind. It is duplicated in DataFrames.jl for a specific case.
Maybe we should define one-argument version in DataAPI.jl instead as this will be the default everywhere I think (at least when someone loads StatsBase.jl it will be anyway)?
Also since we import describe, qualifying it with DataAPI. is not strictly needed (but I am not sure what is a better style to use here).
There was a problem hiding this comment.
So the "type piracy" is explicitly allowed here since DataAPI.jl affirms that StatsBase "owns" this function and provides the default fallback. I agree it may make sense to define this particular fallback in DataAPI, but I also don't think it really matters for DataFrames, since it defines its own describe like describe(x, stat), so it naturally makes sense that it should also define describe(df, stat) = describe(stdout, df, stat) (which is currently does in my PR).
So overall, I understand where you're coming from, but I also worked around in circles a bit here before deciding the current setup should be good (given that StatsBase "owns" describe, which means in general, users who want describe should load StatsBase themselves).
I'd just drop 0.7 altogether. No reason to support it at this point, IMO. |
Codecov Report
@@ Coverage Diff @@
## master #496 +/- ##
==========================================
+ Coverage 90.07% 90.15% +0.07%
==========================================
Files 21 21
Lines 2026 2022 -4
==========================================
- Hits 1825 1823 -2
+ Misses 201 199 -2
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #496 +/- ##
==========================================
+ Coverage 90.07% 90.15% +0.07%
==========================================
Files 21 21
Lines 2026 2022 -4
==========================================
- Hits 1825 1823 -2
+ Misses 201 199 -2
Continue to review full report at Codecov.
|
No description provided.