JuliaData / DataAPI.jl

A data-focused namespace for packages to share functions
Other
33 stars 13 forks source link

add a `groupby` method #64

Closed rafaqz closed 9 months ago

rafaqz commented 10 months ago

Add a groupby method to DataAPI so it can be shared accross packages like DataFrames.jl and DimensionalData.jl.

See: https://github.com/rafaqz/DimensionalData.jl/pull/591

bkamins commented 10 months ago

Also a bump in package version is needed for the release.

codecov[bot] commented 10 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (b1e6855) 96.49% compared to head (102d52f) 96.49%.

:exclamation: Current head 102d52f differs from pull request most recent head 1b8a546. Consider uploading reports for the commit 1b8a546 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #64 +/- ## ======================================= Coverage 96.49% 96.49% ======================================= Files 1 1 Lines 57 57 ======================================= Hits 55 55 Misses 2 2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

nalimilan commented 10 months ago

Does it really make sense to have a docstring that says so little? Also, docstrings are exposed to users, so indications intended only at implementers would better go elsewhere. For join functions (leftjoin , etc.), we currently don't provide any docstring, and instead use a comment inside the code.

bkamins commented 10 months ago

I am OK to turn this into a comment.

rafaqz commented 9 months ago

Any objections to adding aggregate here at the same time for the same reason?

bkamins commented 9 months ago

What packages use aggregate currently? DataFrames.jl does not use this name.

rafaqz commented 9 months ago

Really?

julia> aggregate
WARNING: both Rasters and DataFrames export "aggregate"; uses of it
 in module Main must be qualified

~Maybe a reexport?~ oh its deprecated.

But in practice that amounts to the same thing for users of both packages.

bkamins commented 9 months ago

We will remove aggregate in the next release of DataFrames.jl then.

bkamins commented 9 months ago

OK. @nalimilan - the state is that we export by and aggregate. I think no one uses them, we deprecated them many years ago.

Would you be OK to remove:

export by, aggregate

# TODO: remove definitions in 2.0 release
by(args...; kwargs...) = throw(ArgumentError("by function was removed from DataFrames.jl. " *
                                             "Use the `combine(groupby(...), ...)` or `combine(f, groupby(...))` instead."))
aggregate(args...; kwargs...) = throw(ArgumentError("aggregate function was removed from DataFrames.jl. " *
                                                    "Use the `combine` function instead."))

before 2.0 release (it is technically breaking, but in practice I think it is not problematic to drop it.

nalimilan commented 9 months ago

Sure.

bkamins commented 9 months ago

Released