Repository: JuliaData/Tables.jl (julia)

Issue:
View hint
Currently `subset` takes `view=` which is a hint that may be ignored. I don't like the idea of ignoring keyword argument values, so I'm considering the issue. On Slack some approaches were discussed, 

One option is to rename `view` to `viewhint`. 

Another option is the `view` could be a hint supplemented by a second argument `force` or `strict`. I normally prefer that `force`/`strict` arguments be explicit about what exactly is being forced, for example `forceviewoption`. 

But then once that's the situation, it seems odd to have both `view` and an option for saying "I really mean it" rather than just passing either `viewhint` or `view` with the latter being enforced. The enforced `view` argument could be added in a separate PR, which would make this option very similar to the first.

Therefore I propose renaming `view` to `viewhint`. Then in the future if users so desire, add a `view` argument that would be enforced. They would combine according to the following rules:

```jl
@match (view, viewhint) begin
(true, _) => enforce true
(false, _) => enforce false
(nothing, true) => prefer true
(nothing, false) => prefer false
end
```

An alternative future implementation could define a `subsetview` function if desired.

Pull request description:

Patch:
diff --git a/src/Tables.jl b/src/Tables.jl
index 1bfe17e..a5596c4 100644
--- a/src/Tables.jl
+++ b/src/Tables.jl
@@ -574,7 +574,7 @@ struct Partitioner{T}
 end
 
 """
-    Tables.subset(x, inds; view=nothing)
+    Tables.subset(x, inds; viewhint=nothing)
 
 Return one or more rows from table `x` according to the position(s) specified by `inds`:
 
@@ -584,19 +584,23 @@ Return one or more rows from table `x` according to the position(s) specified by
 
 If other types of `inds` are passed than specified above the behavior is undefined.
 
-The `view` argument influences whether the returned object is a view of the original table
+The `viewhint` argument tries to influence whether the returned object is a view of the original table
 or an independent copy:
 
-- If `view=nothing` (the default) then the implementation for a specific table type
+- If `viewhint=nothing` (the default) then the implementation for a specific table type
   is free to decide  whether to return a copy or a view.
-- If `view=true` then a view is returned and if `view=false` a copy is returned.
+- If `viewhint=true` then a view is returned and if `viewhint=false` a copy is returned.
   This applies both to returning a row or a table.
 
-Any specialized implementation of `subset` must support the `view=nothing` argument.
-Support for `view=true` or `view=false` is optional
-(i.e. implementations may ignore the keyword argument and return a view or a copy regardless of `view` value).
+Any specialized implementation of `subset` must support the `viewhint=nothing` argument.
+Support for `viewhint=true` or `viewhint=false` is optional
+(i.e. implementations may ignore the keyword argument and return a view or a copy regardless of `viewhint` value).
 """
-function subset(x::T, inds; view::Union{Bool, Nothing}=nothing) where {T}
+function subset(x::T, inds; viewhint::Union{Bool, Nothing}=nothing, view::Union{Bool, Nothing}=nothing) where {T}
+    if view !== nothing
+        @warn "`view` keyword argument is deprecated for `Tables.subset`, use `viewhint` instead"
+        viewhint = view
+    end
     # because this method is being called, we know `x` didn't define it's own Tables.subset
     # first check if it supports column access, and if so, apply inds and wrap columns in a DictColumnTable
     if columnaccess(x)
@@ -604,7 +608,7 @@ function subset(x::T, inds; view::Union{Bool, Nothing}=nothing) where {T}
         if inds isa Integer
             return ColumnsRow(cols, inds)
         else
-            ret = view === true ? _map(c -> Base.view(c, inds), cols) : _map(c -> c[inds], cols)
+            ret = viewhint === true ? _map(c -> Base.view(c, inds), cols) : _map(c -> c[inds], cols)
             return DictColumnTable(schema(cols), ret)
         end
     end
@@ -612,7 +616,7 @@ function subset(x::T, inds; view::Union{Bool, Nothing}=nothing) where {T}
     r = rows(x)
     if r isa AbstractVector
         inds isa Integer && return r[inds]
-        ret = view === true ? Base.view(x, inds) : x[inds]
+        ret = viewhint === true ? Base.view(x, inds) : x[inds]
         (ret isa AbstractVector) || throw(ArgumentError("`Tables.subset`: invalid `inds` argument, expected `AbstractVector` output, got $(typeof(ret))"))
         return ret
     end
diff --git a/src/dicts.jl b/src/dicts.jl
index ee5e6db..5b38514 100644
--- a/src/dicts.jl
+++ b/src/dicts.jl
@@ -124,8 +124,12 @@ function Base.iterate(x::DictRowTable, st=1)
     return DictRow(x.names, x.values[st]), st + 1
 end
 
-function subset(x::DictRowTable, inds; view::Union{Bool,Nothing} = nothing)
-    values = view === true ? Base.view(getfield(x, :values), inds) : getfield(x, :values)[inds]
+function subset(x::DictRowTable, inds; viewhint::Union{Bool,Nothing}=nothing, view::Union{Bool,Nothing}=nothing)
+    if view !== nothing
+        @warn "`view` keyword argument is deprecated for `Tables.subset`, use `viewhint` instead"
+        viewhint = view
+    end
+    values = viewhint === true ? Base.view(getfield(x, :values), inds) : getfield(x, :values)[inds]
     if inds isa Integer
         return DictRow(getfield(x, :names), values)
     else
diff --git a/src/namedtuples.jl b/src/namedtuples.jl
index 2245ccc..49e3ea7 100644
--- a/src/namedtuples.jl
+++ b/src/namedtuples.jl
@@ -110,11 +110,15 @@ end
 const ColumnTable = NamedTuple{names, T} where {names, T <: NTuple{N, AbstractVector}} where {N}
 rowcount(c::ColumnTable) = length(c) == 0 ? 0 : length(c[1])
 
-function subset(x::ColumnTable, inds; view::Union{Bool,Nothing}=nothing)
+function subset(x::ColumnTable, inds; viewhint::Union{Bool,Nothing}=nothing, view::Union{Bool,Nothing}=nothing)
+    if view !== nothing
+        @warn "`view` keyword argument is deprecated for `Tables.subset`, use `viewhint` instead"
+        viewhint = view
+    end
     if inds isa Integer
         return map(c -> c[inds], x)
     else
-        return view === true ? map(c -> vectorcheck(Base.view(c, inds)), x) : map(c -> vectorcheck(c[inds]), x)
+        return viewhint === true ? map(c -> vectorcheck(Base.view(c, inds)), x) : map(c -> vectorcheck(c[inds]), x)
     end
 end
 


Test patch:
diff --git a/test/runtests.jl b/test/runtests.jl
index 317af1e..25206bb 100644
--- a/test/runtests.jl
+++ b/test/runtests.jl
@@ -147,30 +147,30 @@ end
 
     @testset "columntable subset" begin
         @test Tables.subset(nt, 1) == (a=1, b=4.0, c="7")
-        @test Tables.subset(nt, 1, view=false) == (a=1, b=4.0, c="7")
-        @test Tables.subset(nt, 1, view=nothing) == (a=1, b=4.0, c="7")
+        @test Tables.subset(nt, 1, viewhint=false) == (a=1, b=4.0, c="7")
+        @test Tables.subset(nt, 1, viewhint=nothing) == (a=1, b=4.0, c="7")
         @test Tables.subset(nt, 1:2) == (a=[1,2], b=[4.0, 5.0], c=["7","8"])
-        @test Tables.subset(nt, 1:2, view=false) == (a=[1,2], b=[4.0, 5.0], c=["7","8"])
-        @test Tables.subset(nt, 1:2, view=nothing) == (a=[1,2], b=[4.0, 5.0], c=["7","8"])
+        @test Tables.subset(nt, 1:2, viewhint=false) == (a=[1,2], b=[4.0, 5.0], c=["7","8"])
+        @test Tables.subset(nt, 1:2, viewhint=nothing) == (a=[1,2], b=[4.0, 5.0], c=["7","8"])
         @test_throws ArgumentError Tables.subset(nt, [1:2 1:2])
 
-        @test Tables.subset(nt, 1, view=true) == (a=1, b=4.0, c="7")
-        rs = Tables.subset(nt, 1:2, view=true)
+        @test Tables.subset(nt, 1, viewhint=true) == (a=1, b=4.0, c="7")
+        rs = Tables.subset(nt, 1:2, viewhint=true)
         @test rs == (a=[1,2], b=[4.0, 5.0], c=["7","8"])
         @test rs.a.parent === nt.a
     end
 
     @testset "rowtable subset" begin
         @test Tables.subset(rt, 1) == (a=1, b=4.0, c="7")
-        @test Tables.subset(rt, 1, view=false) == (a=1, b=4.0, c="7")
-        @test Tables.subset(rt, 1, view=nothing) == (a=1, b=4.0, c="7")
+        @test Tables.subset(rt, 1, viewhint=false) == (a=1, b=4.0, c="7")
+        @test Tables.subset(rt, 1, viewhint=nothing) == (a=1, b=4.0, c="7")
         @test Tables.subset(rt, 1:2) == [(a=1, b=4.0, c="7"), (a=2, b=5.0, c="8")]
-        @test Tables.subset(rt, 1:2, view=false) == [(a=1, b=4.0, c="7"), (a=2, b=5.0, c="8")]
-        @test Tables.subset(rt, 1:2, view=nothing) == [(a=1, b=4.0, c="7"), (a=2, b=5.0, c="8")]
+        @test Tables.subset(rt, 1:2, viewhint=false) == [(a=1, b=4.0, c="7"), (a=2, b=5.0, c="8")]
+        @test Tables.subset(rt, 1:2, viewhint=nothing) == [(a=1, b=4.0, c="7"), (a=2, b=5.0, c="8")]
         @test_throws ArgumentError Tables.subset(rt, [1:2 1:2])
 
-        @test Tables.subset(rt, 1, view=true) == (a=1, b=4.0, c="7")
-        rs = Tables.subset(rt, 1:2, view=true)
+        @test Tables.subset(rt, 1, viewhint=true) == (a=1, b=4.0, c="7")
+        rs = Tables.subset(rt, 1:2, viewhint=true)
         @test rs == [(a=1, b=4.0, c="7"), (a=2, b=5.0, c="8")]
         @test rs.parent === rt
     end
@@ -794,7 +794,7 @@ end
     @test drow.a == 1 && drow.b == 2 && drow.c == 3
     drows = Tables.subset(drt, [1, 2])
     @test length(drows) == 2
-    drowsv = Tables.subset(drt, [1, 2]; view=true)
+    drowsv = Tables.subset(drt, [1, 2]; viewhint=true)
     @test length(drowsv) == 2
 end