search

JuliaData / DataAPI.jl

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

add allcombinations #47

Closed bkamins closed 2 years ago

bkamins commented 2 years ago

add allcombinations function. see https://github.com/JuliaData/DataFrames.jl/pull/3031 for an example implementation.

codecov[bot] commented 2 years ago

Codecov Report

Merging #47 (35723d7) into main (3e905a6) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #47   +/-   ##
=======================================
  Coverage   95.12%   95.12%           
=======================================
  Files           1        1           
  Lines          41       41           
=======================================
  Hits           39       39           
  Misses          2        2
Impacted Files Coverage Δ
src/DataAPI.jl 95.12% <ø> (ø)

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 3e905a6...35723d7. Read the comment docs.

bkamins commented 2 years ago

@nalimilan - now I realized that the approach proposed here (and discussed on slack) cannot be used. The reason is that we cannot dispatch on DataFrame since its type is DataType (it is not a function).

Maybe we should leave https://github.com/JuliaData/DataFrames.jl/pull/3031 as it is then? Or do you see any other solution? The only I can see is that allcombinations would define a table-like type that then would be passed to DataFrame constructor. But then we would need to provide implementation of the logic of the function in DataAPI.jl, which I think we should not do.

nalimilan commented 2 years ago

Can't we just define methods like allcombinations(::Type{DataFrame}, ...)?

bkamins commented 2 years ago

Ah - right. We can 😄. In this case can you please review this PR?

nalimilan commented 2 years ago

@quinnj We could mention in the docstring that TableOperations owns allcombinations(NamedTuple, ...) and implement a method there.

bkamins commented 2 years ago

@quinnj - could you have a look at this PR before I merge it and make a release (and finish https://github.com/JuliaData/DataFrames.jl/pull/3031 a follow-up PR in DataFrames.jl)? Thank you!

bkamins commented 2 years ago

Thank you!