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

remove DataFrame!? #2317

Closed
JeffBezanson opened this issue Jul 17, 2020 · 2 comments · Fixed by #2338
Closed

remove DataFrame!? #2317

JeffBezanson opened this issue Jul 17, 2020 · 2 comments · Fixed by #2338
Labels
breaking The proposed change is breaking. decision
Milestone

Comments

@JeffBezanson
Copy link
Contributor

As far as I can tell, this doesn't mutate anything, so the ! is not appropriate. For the cases where it gives errors, DataFrame could instead give the error if copycols=false is passed.

@bkamins bkamins added breaking The proposed change is breaking. decision labels Jul 17, 2020
@bkamins bkamins added this to the 1.0 milestone Jul 17, 2020
@bkamins
Copy link
Member

bkamins commented Jul 17, 2020

Yes, DataFrame! does not mutate anything.

For the cases where it gives errors,

Having this method is not related with errors - it is just a convenience.

The reason why it exists is that Base Julia does not provide a convenient syntax for partial function application,
and being able to easily avoid copying data when DataFrame is created was considered important by users for performance reasons. The ! in the name was to warn the user that mutation of the source might happen in the future (as in general DataFrame guarantees to perform a copy), as mutation of the source was a major source of bugs in codes using DataFrames.jl.

Now - the major use case for this method was in the past:

CSV.File(filename) |> DataFrame!

which is relevant for very large source files. However, with the changes in JuliaData/CSV.jl#687 this will not be needed.

Therefore, I think we can deprecate DataFrame!, but let us wait for some feedback about this proposal before making a PR.

@quinnj
Copy link
Member

quinnj commented Jul 28, 2020

I think we should deprecate it; I don't think anyone really likes the bang operator here because it's just slightly confusing why it's there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking The proposed change is breaking. decision
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants