-
Notifications
You must be signed in to change notification settings - Fork 13
Add join
family without default definition
#36
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
Conversation
src/DataAPI.jl
Outdated
@@ -183,4 +183,102 @@ default definition. | |||
function unwrap end | |||
unwrap(x) = x | |||
|
|||
""" | |||
innerjoin(x, y, zs...) |
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.
Two small comments:
- Here and below I would drop
zs
, as other packages do not have to provide this. - I think it would be good to mention a reference (or add a description) to what a given type of join does. Ideally (if it is possible to do it relatively easily and in a generic way) - maybe also add a comment that the way key columns are determined is implementation specific and that key equality rules are also implementation specific.
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.
Actually I'm not sure it's very useful to provide docstrings. Packages will have to provide more specific docstrings anyway, and the default docstrings are even misleading as on
may be required by some packages.
For pairwise
in StatsAPI I just added a comment for the same reason. I think that makes sense since that documentation is intended at package authors rather than end users.
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 am OK to fully remove docstrings.
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 remove the docstrings and add some code comments, since it's face to developers.
Codecov Report
@@ Coverage Diff @@
## main #36 +/- ##
=======================================
Coverage 91.30% 91.30%
=======================================
Files 1 1
Lines 23 23
=======================================
Hits 21 21
Misses 2 2
Continue to review full report at Codecov.
|
close JuliaData/Tables.jl#238
Well, I think the default definition isn't trivial to implement, so I just setup the interface functions.
I don't restrict the parameter of join key, which is the kwarg
on
in DataFrames.jl, since the join key is optional in some cases of time series join.