Skip to content

[BREAKING] Deprecate dataframe bang#2338

Merged
bkamins merged 5 commits intoJuliaData:masterfrom
bkamins:deprecate_dataframe_bang
Jul 29, 2020
Merged

[BREAKING] Deprecate dataframe bang#2338
bkamins merged 5 commits intoJuliaData:masterfrom
bkamins:deprecate_dataframe_bang

Conversation

@bkamins
Copy link
Copy Markdown
Member

@bkamins bkamins commented Jul 28, 2020

Fixes #2317

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

bkamins commented Jul 28, 2020

@nalimilan - do you have any take on this? I think it is OK to remove DataFrames!

@deprecate map(f::Pair, gd::GroupedDataFrame) combine(AsTable(first(f)) => last(f), gd, ungroup=false)

@deprecate DataFrame!(args...; kwargs...) begin
if :copycols in keys(kwargs)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Isn't this going to be printed too? I think you need to call depwarn manually. Or just drop this check, people will get an error anyway.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

it is going to be printed. If we remove it then we change the behavior of DataFrame! in some corner cases. The issue is that when someone writes DataFrame!(..., copycols=false) with depwarn you will get:

  1. a warning to use copycols=false
  2. an error that you are not allowed to pass copycols=false

But now as I think of it - I can put depwarn after the error and it will be OK. I will change it

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I the end I decided to skip the error part as it threw error earlier in order to bet a cleaner deprecation.

@bkamins
Copy link
Copy Markdown
Member Author

bkamins commented Jul 28, 2020

should be good now.

@bkamins
Copy link
Copy Markdown
Member Author

bkamins commented Jul 29, 2020

only Coverage fails

@bkamins bkamins merged commit 65ba853 into JuliaData:master Jul 29, 2020
@bkamins bkamins deleted the deprecate_dataframe_bang branch July 29, 2020 15:48
@bkamins
Copy link
Copy Markdown
Member Author

bkamins commented Jul 29, 2020

Thank you!

@bkamins bkamins changed the title Deprecate dataframe bang [BREAKING] Deprecate dataframe bang Aug 7, 2020
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 this pull request may close these issues.

remove DataFrame!?

3 participants