From 148da7bd21bb06f0b8572a85cc1de6832366d5f6 Mon Sep 17 00:00:00 2001
From: pdeffebach <23196228+pdeffebach@users.noreply.github.com>
Date: Tue, 16 Jul 2024 14:37:35 -0400
Subject: [PATCH] Fix keyword argument bug  (#400)

* fix

* progress
---
 NEWS.md                   |  3 +++
 Project.toml              |  2 +-
 src/macros.jl             |  1 -
 src/parsing.jl            | 33 ++++++++++++++++++++++-----------
 test/keyword_arguments.jl | 24 ++++++++++++++++++++++++
 5 files changed, 50 insertions(+), 13 deletions(-)

diff --git a/NEWS.md b/NEWS.md
index 2bec2058..e13c0fb8 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -1,3 +1,6 @@
+# DataFramesMeta v0.15.3 Release Notes
+* Fixed a bug ([#399](https://github.com/JuliaData/DataFramesMeta.jl/issues/399)) where keyword arguments were accidentally ignored ([#400](https://github.com/JuliaData/DataFramesMeta.jl/pull/400#pullrequestreview-2180944667))
+
 # DataFramesMeta v0.15.2 Release notes
 * Bumped the Chain.jl compat entry in the Project.toml ([#382](https://github.com/JuliaData/DataFramesMeta.jl/pull/391))
 
diff --git a/Project.toml b/Project.toml
index 1899e458..4ffdf036 100644
--- a/Project.toml
+++ b/Project.toml
@@ -1,6 +1,6 @@
 name = "DataFramesMeta"
 uuid = "1313f7d8-7da2-5740-9ea0-a2ca25f37964"
-version = "0.15.2"
+version = "0.15.3"
 
 [deps]
 Chain = "8be319e6-bccf-4806-a6f7-6fae938471bc"
diff --git a/src/macros.jl b/src/macros.jl
index 855714ef..acb8ebc1 100644
--- a/src/macros.jl
+++ b/src/macros.jl
@@ -759,7 +759,6 @@ write
 
 function subset_helper(x, args...)
     x, exprs, outer_flags, kw = get_df_args_kwargs(x, args...; wrap_byrow = false)
-
     t = (fun_to_vec(ex; no_dest=true, outer_flags=outer_flags) for ex in exprs)
     quote
         $subset($x, $(t...); (skipmissing = true,)..., $(kw...))
diff --git a/src/parsing.jl b/src/parsing.jl
index a431ec68..e2da6852 100644
--- a/src/parsing.jl
+++ b/src/parsing.jl
@@ -499,22 +499,35 @@ end
 
 function get_df_args_kwargs(x, args...; wrap_byrow = false)
     kw = []
+    # x is normally a data frame. But if the call looks like
+    # transform(df, :x = 1; copycols = false)
+    # then x is actually Expr(:parameters, Expr(:kw, :copycole, false))
+    # When this happens, we assign x to the data frame, use only
+    # the rest of the args, and keep trask of the keyword argument.
     if x isa Expr && x.head === :parameters
         append!(kw, x.args)
         x = first(args)
         args = args[2:end]
     end
 
-    transforms, outer_flags, kw = create_args_vector!(kw, args...; wrap_byrow = wrap_byrow)
+    if args isa Tuple
+        blockarg = Expr(:block, args...)
+    else
+        blockarg = args
+    end
 
-    return (x, transforms, outer_flags, kw)
-end
+    # create_args_vector! has an exclamation point because
+    # we modify the keyword arguments kw
+    transforms, outer_flags, kw = create_args_vector!(kw, blockarg; wrap_byrow = wrap_byrow)
 
-function create_args_vector!(kw, args...; wrap_byrow::Bool=false)
-    create_args_vector!(kw, Expr(:block, args...); wrap_byrow = wrap_byrow)
+    return (x, transforms, outer_flags, kw)
 end
 
 function get_kw_from_macro_call(e::Expr)
+    if length(e.args) != 3
+        throw(ArgumentError("Invalid @kwarg expression"))
+    end
+
     nv = e.args[3]
 
     return nv
@@ -532,8 +545,6 @@ the block as an array. If a simple expression,
 wrap the expression in a one-element vector.
 """
 function create_args_vector!(kw, arg; wrap_byrow::Bool=false)
-    # TODO: Pass vector of keyword arguments to this function
-    # and modify by detecting presence of `@kwarg`.
     arg, outer_flags = extract_macro_flags(MacroTools.unblock(arg))
 
     if wrap_byrow
@@ -545,18 +556,18 @@ function create_args_vector!(kw, arg; wrap_byrow::Bool=false)
     end
 
     # @astable means the whole block is one transformation
+
     if arg isa Expr && arg.head == :block && !outer_flags[ASTABLE_SYM][]
         x = MacroTools.rmlines(arg).args
-        kw = []
         transforms = []
-        seen_kw = false
+        seen_kw_macro = false
         for xi in x
             if is_macro_head(xi, "@kwarg")
                 kw_item = get_kw_from_macro_call(xi)
                 push!(kw, kw_item)
-                seen_kw = true
+                seen_kw_macro = true
             else
-                if seen_kw
+                if seen_kw_macro
                     throw(ArgumentError("@kwarg calls must be at end of block"))
                 end
                 push!(transforms, xi)
diff --git a/test/keyword_arguments.jl b/test/keyword_arguments.jl
index 2d7dd149..0c3116eb 100644
--- a/test/keyword_arguments.jl
+++ b/test/keyword_arguments.jl
@@ -401,4 +401,28 @@ end
     @test df2 == correct
 end
 
+@testset "Multiple arguments #399" begin
+    correct = df[df.a .== 1, :]
+    correct_view = view(df, df.a .== 1, :)
+
+    df2 = @subset(df, :a .== 1, :b .== 3; view = true)
+    @test df2 ≈ correct_view
+
+    @test_throws ArgumentError @subset(df, :a .== 1, :b .== 3; skipmissing = false)
+    @test_throws ArgumentError @subset(df, :a .== 1, :b .== 3; skipmissing = false, view = true)
+
+    correct = transform(df, :a => ByRow(t -> t + 1) => :y, :b => ByRow(t -> t + 2) => :z)
+    df2 = @rtransform(df, :y = :a + 1, :z = :b + 2; copycols = false)
+    @test df2 ≅ correct
+    @test df.a === df2.a
+
+    correct = DataFrame(b_mean = [3.5, 5.0], b_first = [3, 5])
+    df2 = @combine(gd, :b_mean = mean(skipmissing(:b)), :b_first = first(:b); keepkeys = false)
+    @test df2 ≅ correct
+end
+
+@testset "@kwarg errors" begin
+
+end
+
 end # module
\ No newline at end of file