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

make eachcol default to false #1613

Closed
wants to merge 1 commit into from
Closed

Conversation

bkamins
Copy link
Member

@bkamins bkamins commented Dec 3, 2018

This cleans up deprecations and respects JuliaLang/julia#29749.

It would not hurt if someone checked if I had put right Julia version in the condition. Thanks!

@bkamins bkamins changed the title make eachcol default to false and add conditional import from Base make eachcol default to false Dec 4, 2018
@bkamins
Copy link
Member Author

bkamins commented Dec 4, 2018

This should be good for a review. I missed deprecation in mapcols in the #1614; I fix it here - I would leave it as is (the only problem is that mapcols prints a depwarn).

When we merge this then the short-term plan is: #1603 and after it fixing #1608.

@bkamins
Copy link
Member Author

bkamins commented Dec 4, 2018

@nalimilan
The PR is in red, because coverage dropped (probably because I have removed some code and some unnecessary tests). The tests go through in green.

@nalimilan
Copy link
Member

This looks good to me, but I'm not sure we should merge it right now since it will break code that hasn't been ported yet, and there's no hurry to change this: old code that used eachcol(df) will have to do eachcol(df, true) anyway. AFAICT the only advantage of doing this (apart from cleanup) is to allow people to write eachcol(df) instead of eachcol(df, false). Better make a release removing deprecations which people cannot turn off (in indexing code).

@bkamins
Copy link
Member Author

bkamins commented Dec 10, 2018

OK - let us put it on hold. I will keep it open to remember what has to be changed. How releases would you wait with fixing this?

@nalimilan
Copy link
Member

Not sure, but I would at least make a release removing the indexing deprecations first, so that people can upgrade without too many changes to silence them.

@bkamins bkamins mentioned this pull request Jan 15, 2019
31 tasks
@bkamins
Copy link
Member Author

bkamins commented Jan 20, 2019

Do we want to finish the deprecation for eachcol(::DataFrame) or we wait one more release?

@bkamins
Copy link
Member Author

bkamins commented Mar 24, 2019

@nalimilan - I think that enough time has passed for the deprecation. If you are OK to merge it now I will review this PR to make sure and make it ready for merging.

@nalimilan
Copy link
Member

Fine with me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants