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

[BREAKING] deprecate names=true in eachcol #2120

Merged
merged 5 commits into from
Feb 26, 2020

Conversation

bkamins
Copy link
Member

@bkamins bkamins commented Feb 16, 2020

Fixes #2090.

In the next move we will drop V parameter of DataFrameColumns.

Making this PR revealed that names=true is in practice almost never used (at least internally), so this change - while big - should not be that problematic.

@bkamins bkamins added the breaking The proposed change is breaking. label Feb 16, 2020
@bkamins bkamins added this to the 1.0 milestone Feb 16, 2020
@bkamins
Copy link
Member Author

bkamins commented Feb 16, 2020

I decided to redesign eachcol in one shot. What will be breaking:

  • eachcol(df, true) will print differently (it will be a plain array)
  • eachcol(df, true) will not have getproperty and propertynames defined

Given that probably eachcol(df, true) is not used that much and we introduced getproperty only recently I think it should be OK to go this way.

@quinnj
Copy link
Member

quinnj commented Feb 16, 2020

There was a discussion the other day on slack that mentioned a convention of adding [BREAKING] to PRs that introduce breaking changes. This can help later when you review tagbot release notes w/ PRs that start w/ [BREAKING].

@bkamins
Copy link
Member Author

bkamins commented Feb 17, 2020

We now use a label breaking for this. But I can also add [BREAKING] in the name

@bkamins bkamins changed the title deprecate names=true in eachcol [BREAKING] deprecate names=true in eachcol Feb 17, 2020
@bkamins
Copy link
Member Author

bkamins commented Feb 18, 2020

@nalimilan the PR is good to have a look at. the ❌ is due to coveralls only.

src/abstractdataframe/iteration.jl Outdated Show resolved Hide resolved
src/abstractdataframe/iteration.jl Outdated Show resolved Hide resolved
src/abstractdataframe/iteration.jl Outdated Show resolved Hide resolved
src/abstractdataframe/iteration.jl Outdated Show resolved Hide resolved
src/abstractdataframe/iteration.jl Outdated Show resolved Hide resolved
src/abstractdataframe/iteration.jl Outdated Show resolved Hide resolved
test/deprecated.jl Outdated Show resolved Hide resolved
test/deprecated.jl Outdated Show resolved Hide resolved
bkamins and others added 2 commits February 20, 2020 13:34
Co-Authored-By: Milan Bouchet-Valat <nalimilan@club.fr>
@bkamins
Copy link
Member Author

bkamins commented Feb 20, 2020

❌ is due to coverage

@bkamins
Copy link
Member Author

bkamins commented Feb 26, 2020

The failing build on nightly is due to JuliaLang/julia#34889 so I will merge this.

@pablosanjose
Copy link

I just made a PR with a fix for JuliaLang/#34889, sorry about that.

@bkamins
Copy link
Member Author

bkamins commented Feb 26, 2020

@pablosanjose - thank you for such a quick fix.

@bkamins bkamins merged commit 4276bfe into JuliaData:master Feb 26, 2020
@bkamins bkamins deleted the redesign_eachcol branch February 26, 2020 23:06
tpapp added a commit to KristofferC/PGFPlotsX.jl that referenced this pull request Jun 18, 2020
Incidentally, use the faster `reduce(hcat, ...)` form.
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

redesign of eachcol
4 participants