complete update for 0.7 and lots of test fixes #1364

ExpandingMan · 2018-02-26T21:44:29Z

Hello all, I'm not done with this yet but I wanted to put the PR up now so people know that it's being worked on and I have made significant progress.

Apart from the fixes for 0.7 I have noticed that the tests are quite a mess right now. It is very difficult to get 0.7 up and working without clean tests, so I am prioritizing getting the tests nice and spiffy.

I am fixing a huge number of deprecation warnings (that originate in DataFrames.jl) in the tests. I would like to suggest that if we want to keep tests of deprecated functionality, these should be moved to a dedicated test module because otherwise it makes cleaning up tests that are actually problematic rather difficult. This can also cover up real issues. For instance, does getindex require a makeunique keyword? Suppose you do df[:, [1,1,2]] as in the tests? I'll post an issue about this (if there isn't one already).

I may not get to finish this until the end of the week, but I will post updates soon.

Wow, what timing, looks like there will be significant overlap between here and #1363. Perhaps I'll go back and see if I can branch off of that.

nalimilan

Thanks! This looks mostly good, but it can indeed be cleaned/simplified in light of #1364.

nalimilan · 2018-02-26T21:55:34Z

src/other/index.jl

@@ -158,7 +158,7 @@ function Base.getindex(x::AbstractIndex, idx::AbstractVector)
    end
    length(idxs) == 0 && return Int[] # special case of empty idxs
    if idxs[1] isa Real
-        if !all(v -> v isa Integer && !(v isa Bool), idxs)


Why change this?

because it was breaking when a Symbol was passed in

But what changed between 0.6 and 0.7?

Nothing, it was breaking in 0.6 also. Since it only produced a warning nobody seems to have noticed.

@bkamins Does that look OK to you?

It was breaking because we do not allow mixing indexing by number and symbol in the same index. It has to be either all numbers or all symbols. We have to exclude Bool because we do not allow indexing by Bool in this case and Bool <: Integer.

Now conversion to Int will fail on Symbol anyway in line 169.

oh, I think I was very confused about what the intent was there. will change.

nalimilan · 2018-02-26T21:56:18Z

src/other/utils.jl

@@ -46,7 +46,9 @@ function makeidentifier(s::AbstractString)
        end
    end

-    return String(take!(res))
+    # return String(take!(res))
+    # TODO this is to fix a bug in Compat for 0.6


Can you give more details? Is there an issue open in Compat.jl?

see this issue. I'm hoping it will be resolved one way or another before I'm even done with this PR...

nalimilan · 2018-02-26T22:00:25Z

src/DataFrames.jl

+        esc(:(isdefined($sym)))
+    end
+    using Compat.IOBuffer
+    const normalize = normalize_string  # requires that we don't use LinearAlgebra normalize


I'd rather wrap this inside module Unicode, and avoid using Unicode: normalize in the other branch, so that you write Unicode.normalize everywhere to avoid the ambiguity with LinearAlgebra.normalize.

nalimilan · 2018-02-26T22:00:38Z

src/DataFrames.jl

    const kwpairs = pairs
 else
    kwpairs(x::AbstractArray) = (first(v) => last(v) for v in x)
+    macro isdefined(sym)


Would be nice to contribute this to Compat if you have the time.

I did that a while back but it was rejected

yeah, as far as I know @isdefined has been around for quite a while, so I think there's some reason it's not in Compat, though I have no idea what that is. @ararslan , do you think you could find that issue or thread?

JuliaLang/Compat.jl#402

ok, can we perhaps keep it in DataFrames though? I'm trying very hard to put only 0.7 valid code into the main body so that we don't really have to do anything once we no longer support 0.6 (which considering the nature of 0.7 may not be very long)

OK, let's discuss this in Compat. But here I think you can just do isdefined(Base, :unique!), the fact that Base was omitted is most likely an oversight.

ExpandingMan · 2018-02-26T23:41:05Z

Please don't hesitate to merge #1363 on account of this PR. I plan to incorporate those changes as well...

ExpandingMan · 2018-02-26T23:46:29Z

src/DataFrames.jl

@@ -99,6 +108,6 @@ include("dataframerow/show.jl")
 include("abstractdataframe/sort.jl")
 include("dataframe/sort.jl")

-include("deprecated.jl")
+# include("deprecated.jl")


commenting this out was done for testing purposes... will be restored, please ignore

ExpandingMan · 2018-02-27T16:45:19Z

Can I remove code for pre 0.6? see here

ExpandingMan · 2018-02-27T21:58:07Z

Ok, this has been way worse than I was expecting, but I'm slowly getting there.

CategoricalArrays is in pretty bad shape for 0.7, looks like there will be a ton of warnings and even a couple of errors that may have to wait for that.

The IOBuffer issue fix is being pushed to compat, see here.

nalimilan · 2018-02-28T20:59:29Z

src/abstractdataframe/abstractdataframe.jl

@@ -781,6 +782,11 @@ Base.hcat(df1::AbstractDataFrame, df2::AbstractDataFrame, dfn::AbstractDataFrame
    end
 end

+
+# TODO this function to be removed after CategoricalArrays update
+_promote_col_type(::Type{T}, n::Integer) where T = T(uninitialized, n)


What kind of update is needed? Just support for the uninitialized syntax?

(FWIW, use {T} and {T<:...} with one-line function syntax.)

To make the semantics consistent, they should support the T(uninitialized, n) constructor. They don't necessarily have to do that, as far as I know Julia isn't necessarily trying to enforce this as a standard. Of course, I think they should support it. It's a design decision that will have to be made at some point, but for now, here's a work-around.

OK, I've prepared JuliaData/CategoricalArrays.jl#130.

On second thought I'm not even sure defining this constructor for CategoricalArray would be consistent. See my response in your PR.

nalimilan · 2018-02-28T21:03:22Z

src/abstractdataframe/io.jl

@@ -139,10 +139,11 @@ function latex_char_escape(char::AbstractString)
        return string("\\", char)
    end
 end
+# in 0.7 this function will only ever get passed Char, in 0.6 always gets SubString
+latex_char_escape(char::Char) = latex_char_escape(string(char))


Then better put definitions which are not needed on 0.7 under a version check, even if you need to duplicate code. The goal is that in a few months we have the cleanest code possible.

Agreed. 0.6 only works with SubString and 0.7 only works with Char, so I'll have to think about the best way of doing this...

The easiest solution is to have two completely separate blocks, one for each version, and duplicate the code as needed.

nalimilan · 2018-02-28T21:04:09Z

src/dataframe/dataframe.jl

@@ -121,6 +121,8 @@ function DataFrame(pairs::Pair{Symbol,<:Any}...; makeunique::Bool=false)::DataFr
    DataFrame(columns, Index(colnames, makeunique=makeunique))
 end

+DataFrame(dict::Dict{Symbol,<:Any}) = DataFrame((x for x ∈ dict)...)


Why is this needed now?

Oh, this was really interesting I forgot to mention it. 0.7 throws a deprecation warning without this explicit constructor because the "automatic conversion" constructors are deprecated, but for the life of me I have no idea what the Dict constructor actually wound up calling before. My first instinct was to do away with the Pair constructor and have it feed into a Dict constructor, but I had to do it the other way around because of makeunique.

OK. Then Base.convert(::Type{DataFrame}, d::AbstractDict) was called automatically on 0.6. On 0.7, the recommendation added in JuliaLang/julia#23273 is to define constructors, and have convert methods call constructors explicitly (so the inverse of what we do now, mostly).

(Note that splatting should generally be avoided when the number of elements can be large, as is the case here: it's much more efficient to pass the collection and loop over it inside the function.)

nalimilan · 2018-02-28T21:04:55Z

src/other/index.jl

@@ -158,7 +158,7 @@ function Base.getindex(x::AbstractIndex, idx::AbstractVector)
    end
    length(idxs) == 0 && return Int[] # special case of empty idxs
    if idxs[1] isa Real
-        if !all(v -> v isa Integer && !(v isa Bool), idxs)


But what changed between 0.6 and 0.7?

nalimilan · 2018-02-28T21:06:00Z

test/cat.jl

    #
    # hcat
    #

    nvint = [1, 2, missing, 4]
    nvstr = ["one", "two", missing, "four"]

-    df2 = DataFrame(Any[nvint, nvstr])


These changes are no longer needed now.

nalimilan · 2018-02-28T21:15:10Z

test/show.jl

@@ -1,12 +1,19 @@
 module TestShow
    using Compat, Compat.Test, DataFrames

+    if VERSION ≥ v"0.7.0-"
+        using Random


Use Compat.Random. Instead of dataframe_str, I think you can just do $DataFrame inside strings.

nalimilan · 2018-02-28T21:16:59Z

test/utils.jl

@@ -1,5 +1,11 @@
 module TestUtils
    using Compat, Compat.Test, DataFrames, StatsBase
+    if VERSION ≤ v"0.7.0-"
+        using Compat.Random


Doesn't need to be under a version check. Also use Compat.@warn.

nalimilan · 2018-02-28T21:17:13Z

test/utils.jl

    # @test_throws ArgumentError DataFrames.make_unique([:x, :x, :x_1, :x2], makeunique=false)
-    @test DataFrames.make_unique([:x, :x_1, :x2], makeunique=false) == [:x, :x_1, :x2]
+    # TODO below is commented out for now for warning sanity


lol, I don't even remember... will change

nalimilan · 2018-02-28T21:17:31Z

test/utils.jl

@@ -30,7 +37,7 @@ module TestUtils
            @test rw == DataFrames.RESERVED_WORDS
        end
    else
-        warn("Unable to validate reserved words against parser. ",
+        @warn("Unable to validate reserved words against parser. ",


Incorrect indentation.

nalimilan · 2018-02-28T21:18:36Z

test/utils.jl

@@ -150,6 +157,8 @@ module TestUtils

            """
            out = String(take!(io))
+            # TODO this test is very sensitive to irrelevant details of show implementations 


Note that you can use $Missing and $CategoricalValue to include some variations automatically.

ExpandingMan · 2018-02-28T21:32:04Z

Ok! I have cleaned everything up the best I think is currently possible. Some comments:

CategoricalArrays and StatsBase are both still badly screwed up on 0.7. There are a number of 0.7 test errors and deprecation warnings resulting from CategoricalArrays. If I have time I'm going to clean up these packages also.
All 0.6 tests now pass without any errors or unexpected warnings. I have moved all tests of deprecated functionality to the new test file "deprecated.jl". I very much would like to keep all tests of deprecated functionality separate, as it becomes very difficult to parse the test results on 0.7 when there are lots of deliberate deprecation warnings thrown in.
The IOBuffer bug on Compat has been fixed so I have removed my hack.

Hopefully we can avoid master having DataFrames related test errors in the future? 😉

Tagging @nalimilan , please let me know any changes you'd like me to make so we can get this merged.

ExpandingMan · 2018-03-01T15:15:33Z

Hello all, I'd like to make a general plea that warnings be taken more seriously in the future.

Getting this going on 0.7 was surprisingly painful and that was much exacerbated by the many warnings already appearing in the tests under normal circumstances. Not only to these make updates more difficult, but they can hide real issues, a couple of examples of which can be seen in this PR. This also makes it a lot harder to notice legitimate warnings from Base Julia concerning needed changes and warnings that might be inappropriately given to users. Lastly, the warnings make it more difficult for people to contribute to the package, since it can be hard to figure out what then intent of some of the tests and warnings are (as it sometimes was for me).

Note that I have moved all tests of deprecated functionality which deliberately produce warnings to a separate deprecated.jl test file, which should make it easier to filter out some of the intended warnings.

bkamins · 2018-03-01T16:56:05Z

src/dataframe/dataframe.jl

@@ -121,6 +121,8 @@ function DataFrame(pairs::Pair{Symbol,<:Any}...; makeunique::Bool=false)::DataFr
    DataFrame(columns, Index(colnames, makeunique=makeunique))
 end

+DataFrame(dict::Dict{Symbol,<:Any}) = convert(DataFrame, dict)


Should it be Dict or AbstractDict?

bkamins · 2018-03-01T17:02:10Z

src/other/index.jl

@@ -158,7 +158,7 @@ function Base.getindex(x::AbstractIndex, idx::AbstractVector)
    end
    length(idxs) == 0 && return Int[] # special case of empty idxs
    if idxs[1] isa Real
-        if !all(v -> v isa Integer && !(v isa Bool), idxs)


It was breaking because we do not allow mixing indexing by number and symbol in the same index. It has to be either all numbers or all symbols. We have to exclude Bool because we do not allow indexing by Bool in this case and Bool <: Integer.

Now conversion to Int will fail on Symbol anyway in line 169.

bkamins · 2018-03-01T17:04:32Z

test/dataframe.jl

-    z = vcat(v, x)
-
-    # TODO: uncomment the line below after deprecation
-    # @test_throws ArgumentError z[:, [1, 1, 2]]


Why this line (61 and 62) is removed? I think we should test that it throws an error after deprecation.

Yes, but currently this is not fully deprecated behavior so it does not yet throw an error. When automatic generation of column names from indexing is fully deprecated, this line should be un-commented. Note that we test the current behavior in deprecation.jl.

Better keep this commented out then.

bkamins · 2018-03-01T17:08:53Z

test/index.jl

@@ -13,11 +14,11 @@ inds = Any[1,
           [big(1)],
           big(1):big(1),
           [:A],
-           Union{Bool, Missing}[true, false],


On my machine it does not throw deprecation warning. The vector does not contain any missing and getindex for vector of Union{Bool, Missing} tests for it. And this specific case is not tested in "deprecated.jl".

bkamins · 2018-03-01T17:12:29Z

@ExpandingMan Huge work. Thank you. I have left my comments inline.

ExpandingMan · 2018-03-01T18:21:54Z

Think I've complied with all comments now, so we should be good.

Glad to help! I have a few things that used DataFrames that I was eager to get running on 0.7, so this seemed fairly high priority.

Note that I still get a non-uninitialized constructor warning when I compile DataFrames on 0.7 and have absolutely no idea where it's coming from (annoyingly doesn't give any traceback)!

CategoricalArrays seems like it's in much worse shape. I hope to at least make enough fixes to that so that it doesn't screw everything else up, including DataFrames (currently we still have failing tests on 0.7).

nalimilan · 2018-03-01T20:19:23Z

I agree that warnings are problematic in general. I don't like warnings because they indicate something is wrong, yet no failure happens. Ideally we would turn deprecations from DataFrames into errors (but not those from other packages), like Julia does, but I don't think that's possible currently, and tests for deprecated features would not be possible.

nalimilan · 2018-03-01T19:27:07Z

test/cat.jl

        @test df[1] == CategoricalVector(1:10)
        DataFrames.hcat!(df, 1:10, makeunique=true)
        @test df[2] == collect(1:10)
        DataFrames.hcat!(df, collect(1:10), makeunique=true)
+


No need for this new line break.

nalimilan · 2018-03-01T19:30:53Z

test/cat.jl

@@ -51,23 +51,27 @@ module TestCat

    @testset "hcat ::AbstractVectors" begin
        df = DataFrame()
-        DataFrames.hcat!(df, CategoricalVector{Union{Int, Missing}}(1:10))
+        dfb = DataFrame(x1=CategoricalVector{Union{Int,Missing}}(1:10))


Is this change needed? Same below. Please revert the name changes.

nalimilan · 2018-03-01T19:33:17Z

test/data.jl

-    using Compat, Compat.Test, DataFrames
-    importall Base # so that we get warnings for conflicts
+    using Compat, Compat.Test, DataFrames, Compat.Random
+    # TODO this already generates a huge wall of warnings


Leaving a note that something needs to be done about this.

nalimilan · 2018-03-01T19:33:39Z

test/data.jl

-    df3 = DataFrame(Any[[1, 2, missing, 4]])
-    df4 = DataFrame(Any[Vector{Union{Int, Missing}}(1:4), Vector{Union{Int, Missing}}(1:4)])
-    df5 = DataFrame(Any[Union{Int, Missing}[1, 2, 3, 4], ["one", "two", missing, "four"]])
+    df2 = DataFrame(Any[[1, 2, missing, 4], ["one", "two", missing, "four"]], [:x1, :x2])


These are no longer needed AFAICT. Same below in several places.

nalimilan · 2018-03-01T19:35:02Z

test/dataframe.jl

-    z = vcat(v, x)
-
-    # TODO: uncomment the line below after deprecation
-    # @test_throws ArgumentError z[:, [1, 1, 2]]


Better keep this commented out then.

nalimilan · 2018-03-01T19:40:44Z

test/join.jl

-    @test join(nameid, jobid, on = :ID, kind = :right) == right[on]
-    @test join(nameid, jobid, on = :ID, kind = :semi) == semi[on]
-    @test join(nameid, jobid, on = :ID, kind = :anti) == anti[on]
+    on_ = [:ID]


Please avoid renaming variables.

All such renames are in order to avoid extraneous warnings. Do you want me to revert to warnings?

Yes, as I said I prefer keeping the code clean and wait until the (spurious) warnings go away than living with weird variable names forever.

nalimilan · 2018-03-01T19:44:47Z

test/join.jl

@@ -71,63 +72,56 @@ module TestJoin
    # Cross joins don't take keys
    @test_throws ArgumentError join(df1, df2, on = :A, kind = :cross)

-    @testset "Test empty inputs 1" begin


Yeah, but these are the same warnings as when reusing object names, we don't care since we don't want to overwrite the method. Please keep this.

nalimilan · 2018-03-01T19:46:53Z

test/utils.jl

            Number Unique:  4
            Number Missing: 0
            % Missing:      0.000000

            """
            out = String(take!(io))
-            @test (out == missingfirst || out == missingsecond)
+            # TODO this test is very sensitive to irrelevant details of show implementations 


Better remove this comment.

nalimilan · 2018-03-01T19:48:26Z

test/utils.jl

@@ -66,7 +67,7 @@ module TestUtils
        describe(io, df)
        DRT = CategoricalArrays.DefaultRefType
        # Julia 0.7


There's nothing 0.7-specific AFAICT. Same below.

nalimilan · 2018-03-01T19:58:34Z

src/abstractdataframe/abstractdataframe.jl

@@ -781,6 +782,11 @@ Base.hcat(df1::AbstractDataFrame, df2::AbstractDataFrame, dfn::AbstractDataFrame
    end
 end

+
+# TODO this function to be removed after CategoricalArrays update


Once JuliaLang/METADATA.jl#13598 is merged, please revert this, require CategoricalArrays 0.3.5, and also adjust uses of the current constructors, e.g. in allocate.

bkamins · 2018-03-01T23:57:37Z

@ExpandingMan Regarding your question about df[:, [1,1,2]] I think that getindex does not need makeunique keyword argument. Such duplicate columns are aliases and I do not see much use for such indexing, but I might not see something so it is not a strong no-go.

ExpandingMan · 2018-03-01T23:59:52Z

Yeah, my confusion has cleared up a bit. It seems that df[:, [1,1,2]] is deprecated, currently it throws a warning, and it will throw an error in the future. There is already a test of the error that should be uncommented when the full deprecation is enacted.

bkamins · 2018-03-02T00:00:53Z

Exactly! Thanks

nalimilan

Thanks, I think it's almost ready after fixing the things I've noted. Just remove all the unnecessary churn due to renaming variables, which should make the PR much more readable.

nalimilan · 2018-03-02T13:53:17Z

test/data.jl

    const ≅ = isequal

    #test_group("constructors")
    df1 = DataFrame(Any[[1, 2, missing, 4], ["one", "two", missing, "four"]], [:Ints, :Strs])
    df2 = DataFrame(Any[[1, 2, missing, 4], ["one", "two", missing, "four"]])
-    df3 = DataFrame(Any[[1, 2, missing, 4]])
+    df3 = DataFrame(Any[[1, 2, missing, 4]], [:x1])


Not needed.

nalimilan · 2018-03-02T13:53:50Z

test/data.jl

-    using Compat, Compat.Test, DataFrames
-    importall Base # so that we get warnings for conflicts
+    using Compat, Compat.Test, DataFrames, Compat.Random
+    # TODO: need to find a better way of accomplishing below


Leaving another note since the previous one has been closed for some reason. Please either keep this or remove it, but adding comments doesn't help fixing the problem.

You want me to retain the importall or remove it?

I'd say remove it, anyway the comment probably referred to ambiguity warnings, which are no longer printed.

nalimilan · 2018-03-02T13:55:00Z

src/DataFrames.jl

    const kwpairs = pairs
 else
    kwpairs(x::AbstractArray) = (first(v) => last(v) for v in x)
+    using Compat.IOBuffer
+    import Base.LinAlg: normalize


As I suggested before, what happens if you remove this line?

There was a Compat update so I was really hoping I could do this properly, but it's still really screwed up. When you do using Compat.Unicode: normalize in 0.6 it doesn't import the normalize_str methods. Anyway, this can't be removed, because it throws an error telling you that you must explicitly import LinAlg.normalize. Sorry I've been hesitant to make PR's to Compat, but there are so many gotchas and existing bugs that doing that is rather difficult.

This should not break if Compat gets fixed, especially because Unicode doesn't even export normalize.

OK. Doesn't it work if you do using Compat.Unicode and then Unicode.normalize for every use?

BTW, you should be able to use it soon (don't forget to update REQUIRE): JuliaLang/METADATA.jl#13615

awesome, thanks, I will wait until METADATA gets updated and try it out.

nalimilan · 2018-03-02T13:56:51Z

test/dataframe.jl

@@ -522,7 +508,7 @@ module TestDataFrame
    end

    @testset "misc" begin
-        df = DataFrame(Any[collect('A':'C')])
+        df = DataFrame(Any[collect('A':'C')], [:x1])


Not needed (same below).

nalimilan · 2018-03-02T14:04:03Z

src/dataframe/dataframe.jl

@@ -121,6 +121,8 @@ function DataFrame(pairs::Pair{Symbol,<:Any}...; makeunique::Bool=false)::DataFr
    DataFrame(columns, Index(colnames, makeunique=makeunique))
 end

+DataFrame(dict::AbstractDict) = convert(DataFrame, dict)


You appear to have missed my previous comment.

? Wasn't it regarding splatting? This is no longer splatting. Can you explain what you'd like me to change here?

I mean this:

On 0.7, the recommendation added in JuliaLang/julia#23273 is to define constructors, and have convert methods call constructors explicitly (so the inverse of what we do now, mostly).

Oh, sorry I had missed that. Will change now.

nalimilan · 2018-03-02T14:08:26Z

src/abstractdataframe/io.jl

-        return "\\textasciitilde{}"
-    else
-        return string("\\", char)
+if VERSION ≥ v"0.7.0-"


That's 0.7.0-DEV.4059.

nalimilan · 2018-03-02T17:14:43Z

test/dataframe.jl

    x = DataFrame(a = [1, 2, 3], b = [4, 5, 6])
-    v = DataFrame(a = [5, 6, 7], b = [8, 9, 10])


Still need to do something about this: https://github.com/JuliaData/DataFrames.jl/pull/1364/files#r171626211

I did, it's in deprecated.jl now. This dataframe v is removed here because it is no longer used in this file (appears again in deprecated.jl).

nalimilan · 2018-03-02T17:16:41Z

test/cat.jl

        @test df[1] == CategoricalVector(1:10)
        DataFrames.hcat!(df, 1:10, makeunique=true)
        @test df[2] == collect(1:10)
        DataFrames.hcat!(df, collect(1:10), makeunique=true)
        @test df[3] == collect(1:10)

        df = DataFrame()
-        df2 = hcat(CategoricalVector{Union{Int, Missing}}(1:10), df)


Can you revert all the renames in this file and the following files?

Yes, don't worry, I'm not ignoring you, I was busy...

No worries. I leave new comments because GitHub hides the old ones and it's hard to keep track.

Yeah, I also never know if you are aware that I made changes, though by now it's seeming that you must get a notification whenever I commit...

bkamins

I send approve not to block merging when @nalimilan is OK with the PR (but I will try to review the final version of changes again if I will have time).

nalimilan · 2018-03-02T18:53:24Z

test/grouping.jl

-        df = DataFrame(Key1 = ["A", missing, "B", "B", "A"],
-                       Key2 = CategoricalArray(["B", "A", "A", missing, "A"]),
-                       Value = 1:5)
+        df= DataFrame(Key1 = ["A", missing, "B", "B", "A"],


Wrong indentation.

nalimilan · 2018-03-02T18:59:23Z

test/show.jl

    │ Col # │ Name │ Eltype                           │ Missing │ Values          │
    ├───────┼──────┼──────────────────────────────────┼─────────┼─────────────────┤
    │ 1     │ Fish │ String                           │ 0       │ Suzy  …  Amir   │
-    │ 2     │ Mass │ Union{Float64, Missings.Missing} │ 1       │ 1.5  …  missing │"""
+    │ 2     │ Mass │ Union{Float64, $Missing} │ 1       │ 1.5  …  missing │"""


I've just realized this probably won't work on 0.7, where the Missing prefix won't be present, as the other columns will be less wide than in the string. So I guess we'll need two blocks with a VERSION check. There's another case below.

good catch!

ExpandingMan · 2018-03-02T21:03:46Z

Thanks for pushing that last commit. Sorry, as this wraps up I'm starting to lose track of things.

nalimilan

Thanks, look good now!

ExpandingMan · 2018-03-02T21:15:57Z

Yay! In the future if I ever do something this big I'll probably split it into multiple PR's. I suspect that might save us all a bit of sanity.

nalimilan · 2018-03-02T21:43:46Z

Hmm, I wonder why some lines are not covered by tests anymore. For example, compare:
https://coveralls.io/builds/15784016/source?filename=src%2Fdataframe%2Fdataframe.jl#L403
with the last commit on master:
https://coveralls.io/builds/15401106/source?filename=src%2Fdataframe%2Fdataframe.jl#L403

EDIT: scratch that, the links I posted actually show that the regression is older.
@bkamins Any idea which PR could have introduced this regression in coverage? I couldn't find it, but on 0.11.5 that line and others were covered: https://coveralls.io/builds/15131504/source?filename=src%2Fdataframe%2Fdataframe.jl#L403

EDIT: filed #1372

ExpandingMan · 2018-03-02T21:48:58Z

That was added for a reason, though now I don't remember exactly what that was. I expect this to break something.

nalimilan · 2018-03-02T21:58:51Z

Yeah, there's a deprecation warning about it, but let's handle this in the next round of fixes. Else we need more tests, which is going to hold off the PR.

ExpandingMan · 2018-03-02T22:05:24Z

Oh! I remember what it fixes.

Without it you can't do e.g. df[1:end, 1].

nalimilan · 2018-03-02T22:11:55Z

Let's handle this in another PR, anyway we're not there yet!

ExpandingMan · 2018-03-02T22:14:27Z

Acknowledged, opened an issue so we don't forget.

Lots of compatibility and test fixes.

41a7e3c

nalimilan reviewed Feb 26, 2018

View reviewed changes

ExpandingMan commented Feb 26, 2018

View reviewed changes

ExpandingMan added 2 commits February 27, 2018 12:14

0.6 tests now run with no errors or unexpected warnings.

2f93874

Further fixes for 0.7. Broke 0.6 again.

17454e8

ExpandingMan and others added 5 commits February 28, 2018 10:37

0.6 is back to having no errors.

3e2baba

All done (for now).

188cdde

Added changes from test fixing PR.

e178a63

Merge branch 'master' into 0p7fix

0c78964

Cleaned up deprecated tests.

b88f185

nalimilan reviewed Feb 28, 2018

View reviewed changes

ExpandingMan added 2 commits March 1, 2018 10:02

Various fixes and updates.

71b477c

Fix for merge.

3a1fee4

Removed excessive globals.

06d1783

bkamins requested changes Mar 1, 2018

View reviewed changes

Small fixes.

0353a5e

nalimilan reviewed Mar 1, 2018

View reviewed changes

More requested changes.

3a44016

nalimilan reviewed Mar 2, 2018

View reviewed changes

nalimilan mentioned this pull request Mar 2, 2018

should getindex type semantics be cleaned up? #1369

Closed

ExpandingMan added 2 commits March 2, 2018 11:36

Lots of small requested changes, finally fixed Unicode.normalize issue.

3427e5e

Small fixes to reduce churn.

93cc70f

nalimilan reviewed Mar 2, 2018

View reviewed changes

bkamins approved these changes Mar 2, 2018

View reviewed changes

Reverted variable name changes in test.

57f86f3

nalimilan reviewed Mar 2, 2018

View reviewed changes

ExpandingMan and others added 3 commits March 2, 2018 14:59

Show test fixes.

c6d6198

fixed silly indentation typo

612bfa0

Make version more precise

28039a9

nalimilan approved these changes Mar 2, 2018

View reviewed changes

Remove untested new lastindex method

d2a60c4

nalimilan merged commit c09b880 into JuliaData:master Mar 2, 2018

		x = DataFrame(a = [1, 2, 3], b = [4, 5, 6])
		v = DataFrame(a = [5, 6, 7], b = [8, 9, 10])

complete update for 0.7 and lots of test fixes #1364

complete update for 0.7 and lots of test fixes #1364

Conversation

ExpandingMan commented Feb 26, 2018 • edited Loading

nalimilan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ExpandingMan commented Feb 26, 2018

Choose a reason for hiding this comment

ExpandingMan commented Feb 27, 2018

ExpandingMan commented Feb 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ExpandingMan commented Feb 28, 2018

ExpandingMan commented Mar 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkamins commented Mar 1, 2018

ExpandingMan commented Mar 1, 2018

nalimilan commented Mar 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkamins commented Mar 1, 2018

ExpandingMan commented Mar 1, 2018

bkamins commented Mar 2, 2018

nalimilan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ExpandingMan commented Feb 26, 2018 •

edited

Loading