Skip to content
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

Proposal for gen and collapse #2210

Closed
wants to merge 3 commits into from
Closed

Conversation

pdeffebach
Copy link
Contributor

This is a very naive implementation of @bkamins 's plan for collapse, etc. described here.

Please keep comments focused on #2206. This is just to demonstrate how I imagine this should work.

@pdeffebach pdeffebach mentioned this pull request Apr 25, 2020
@bkamins
Copy link
Member

bkamins commented Apr 25, 2020

OK - I treat it as a reference to a discussion. This PR should not be merged and will be closed when #2206 is resolved.

@pdeffebach
Copy link
Contributor Author

I will update this PR as consensus develops. When we all agree we can implement it for real.

@bkamins
Copy link
Member

bkamins commented Apr 25, 2020

OK, but as @nalimilan commented in #2206 I do not expect that we will allow to define so many functions in DataFrames.jl. We will rather keep a minimal list of functions that cover all required functionality. The functions you propose are good for an extension package to DataFrames.jl so that users can opt-in to use them.

@@ -0,0 +1,46 @@
function gen(df::AbstractDataFrame, args...; kwargs...)
transform(df, args...; kwargs...)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

first small note (just for the future discussions) - this is not a correct implementation as args could overwrite all source columns of df.

end

function keep(df::AbstractDataFrame, args...; kwargs...)
t = gensym()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a second small note gensym() cannot be used here as it does not guarantee we will not have column name conflicts. But again - as above this is a minor issue than can be fixed.

@bkamins
Copy link
Member

bkamins commented May 7, 2020

I am closing this as 0.21 was released. If you decide to put these definitions in some accompanying package ping me and I can help with the review. Thank you for working on this!

@bkamins bkamins closed this May 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants