Handle keyword arguments separately

[kyohsuke]

Hi, amatsuda

I got this warning on ruby 2.7.x and want to remove it.

warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call

If the patch is no problem, could you please merge them?

Thanks.

[kamipo]

The keyword argument warning is set of two lines.

For example https://github.com/rails/rails/pull/39199, without ruby2_keywords, test shows the following warnings:

diff --git a/activerecord/lib/active_record/scoping/named.rb b/activerecord/lib/active_record/scoping/named.rb
index e2167881e5e9..8ffdadae5420 100644
--- a/activerecord/lib/active_record/scoping/named.rb
+++ b/activerecord/lib/active_record/scoping/named.rb
@@ -195,6 +195,7 @@ def scope(name, body, &block)
               scope
             end
           end
+          singleton_class.send(:ruby2_keywords, name) if respond_to?(:ruby2_keywords, true)

           generate_relation_method(name)
         end
diff --git a/activerecord/test/cases/scoping/named_scoping_test.rb b/activerecord/test/cases/scoping/named_scoping_test.rb
index c045ae5e8ea1..232e437be96b 100644
--- a/activerecord/test/cases/scoping/named_scoping_test.rb
+++ b/activerecord/test/cases/scoping/named_scoping_test.rb
@@ -127,6 +127,18 @@ def test_scope_with_object
     assert objects.all?(&:approved?), "all objects should be approved"
   end

+  def test_scope_with_kwargs
+    # Explicit true
+    topics = Topic.with_kwargs(approved: true)
+    assert_operator topics.length, :>, 0
+    assert topics.all?(&:approved?), "all objects should be approved"
+
+    # No arguments
+    topics = Topic.with_kwargs()
+    assert_operator topics.length, :>, 0
+    assert topics.none?(&:approved?), "all objects should not be approved"
+  end
+
   def test_has_many_associations_have_access_to_scopes
     assert_not_equal Post.containing_the_letter_a, authors(:david).posts
     assert_not_empty Post.containing_the_letter_a
diff --git a/activerecord/test/models/topic.rb b/activerecord/test/models/topic.rb
index 759d21aa3ef4..a3a0f570aee6 100644
--- a/activerecord/test/models/topic.rb
+++ b/activerecord/test/models/topic.rb
@@ -32,6 +32,8 @@ def call
     end
   }.new(self)

+  scope :with_kwargs, ->(approved: false) { where(approved: approved) }
+
   module NamedExtension
     def two
       2

% bin/test test/cases/scoping/named_scoping_test.rb -n test_scope_with_kwargs
Using sqlite3
Run options: -n test_scope_with_kwargs --seed 4156

# Running:

/Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/relation.rb:412: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/Users/kamipo/src/github.com/rails/rails/activerecord/test/models/topic.rb:35: warning: The called method is defined here
.

Finished in 0.053403s, 18.7255 runs/s, 74.9022 assertions/s.
1 runs, 4 assertions, 0 failures, 0 errors, 0 skips

The warning shows with_kwargs method in test/models/topic.rb:35 is called at lib/active_record/relation.rb:412 without ** or kwargs flagged hash, but lib/active_record/relation.rb:412 is not the cause of the warning.

lib/active_record/relation.rb:412 is called at lib/active_record/scoping/named.rb:187, so in this case we should fix the warning in lib/active_record/scoping/named.rb.

[kamipo]

What I say in the above comment, the description in this PR has few information, it is harder to understand where is the cause.

% git grep -n 'def execute\b' lib/active_record/connection_adapters
lib/active_record/connection_adapters/abstract/database_statements.rb:118:      def execute(sql, name = nil)
lib/active_record/connection_adapters/abstract_mysql_adapter.rb:192:      def execute(sql, name = nil)
lib/active_record/connection_adapters/mysql/database_statements.rb:41:        def execute(sql, name = nil)
lib/active_record/connection_adapters/postgresql/database_statements.rb:36:        def execute(sql, name = nil)
lib/active_record/connection_adapters/sqlite3/database_statements.rb:21:        def execute(sql, name = nil) #:nodoc:

% git grep -n 'def exec_query\b' lib/active_record/connection_adapters
lib/active_record/connection_adapters/abstract/database_statements.rb:125:      def exec_query(sql, name = "SQL", binds = [], prepare: false)
lib/active_record/connection_adapters/mysql/database_statements.rb:53:        def exec_query(sql, name = "SQL", binds = [], prepare: false)
lib/active_record/connection_adapters/postgresql/database_statements.rb:50:        def exec_query(sql, name = "SQL", binds = [], prepare: false)
lib/active_record/connection_adapters/sqlite3/database_statements.rb:35:        def exec_query(sql, name = nil, binds = [], prepare: false)

I've investigated in the Rails code base, I suppose the fix for exec_query would be a right direction, but looks there is no need to change execute.

[amatsuda]

@kamipo Thanks for the input!

@kyohsuke We'd better not change the execute method signature, and we don't need to touch any given arguments or options in exec_query. So, couldn't the patch just be like this?

-    def exec_query(sql, *)
+    def exec_query(sql, *, **)

[kyohsuke]

@kamipo Thanks for the assertion.

@amatsuda

We'd better not change the execute method signature, and we don't need to touch any given arguments or options in exec_query. So, couldn't the patch just be like this?

-    def exec_query(sql, *)
+    def exec_query(sql, *, **)

I've changed it to your indicated. But it breaks the test for older versions.

[amatsuda]

Hm... maybe we need to check the adapter's exec_query's arity, or we can just check the Rails version?

[kyohsuke]

I found the ruby 2.7.x fail as a consequence of rubygems-update's version. I changed it to the following and it worked. https://github.com/kyohsuke/database_rewinder/commit/65883094f5342c572bee4cc20f84dd7c937ae2c4

[amatsuda]

@kyohsuke Oh, thank you for the fix! Could you either add that commit to this PR or create another PR?

[kyohsuke]

Could you either add that commit to this PR or create another PR?

Sure! I'm writing it now.

[kyohsuke]

I got it. def exec_query has different arguments between rails 5 and 4.

$ git checkout v4.2.11.1
$ git grep -n 'def exec_query\b' activerecord/lib/active_record/connection_adapters

activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:69:      def exec_query(sql, name = 'SQL', binds = [])
activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb:220:      def exec_query(sql, name = 'SQL', binds = [])
activerecord/lib/active_record/connection_adapters/mysql_adapter.rb:253:      def exec_query(sql, name = 'SQL', binds = [])
activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb:159:        def exec_query(sql, name = 'SQL', binds = [])
activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb:276:      def exec_query(sql, name = nil, binds = [])

$ git checkout v5.2.4
$ git grep -n 'def exec_query\b' activerecord/lib/active_record/connection_adapters

activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:121:      def exec_query(sql, name = "SQL", binds = [], prepare: false)
activerecord/lib/active_record/connection_adapters/mysql/database_statements.rb:31:        def exec_query(sql, name = "SQL", binds = [], prepare: false)
activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb:80:        def exec_query(sql, name = "SQL", binds = [], prepare: false)
activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb:209:      def exec_query(sql, name = nil, binds = [], prepare: false)

I can fix it in the following way, but I think it's not a good way to do it. Is there any better way to do it?

https://github.com/kyohsuke/database_rewinder/compare/100c248bab16...3b05cf3575cc Travis CI

[amatsuda]

@kyohsuke Right. That's what I meant by "check the Rails version", and in this case I prefer sth like this so we can reduce extra runtime overhead:

if Rails::VERSION::MAJOR < 5
  def exec_query(sql, *)
    ...
  end
else
  def exec_query(sql, *, **)
    ...
  end
end

[kyohsuke]

That's what I meant by "check the Rails version"

I'm sorry, I understand now.

[amatsuda]

@kyohsuke Or if you don't like hardcoding the Rails version, here's another possible approach (the one that I said "check the arity"):

module DatabaseRewinder
  module InsertRecorder
    def self.included(mod)
      if mod.instance_method(:exec_query).parameters.any? {|p| p.first == :key }
        mod.send :prepend, WithKwargs
      else
        mod.send :prepend, WithoutKwargs
      end
    end

    module WithoutKwargs
      def execute(sql, *)
        ...

      def exec_query(sql, *)
        ...

    module WithKwargs
      def execute(sql, *)
        ...

      def exec_query(sql, *, **)
        ...

::ActiveRecord::ConnectionAdapters::SQLite3Adapter.send :include, DatabaseRewinder::InsertRecorder if defined? ::ActiveRecord::ConnectionAdapters::SQLite3Adapter
...

or

module DatabaseRewinder
  module InsertRecorder
    def self.patch(adapter)
      if adapter.instance_method(:exec_query).parameters.any? {|p| p.first == :key }
        adapter.send :prepend, WithKwargs
      else
        adapter.send :prepend, WithoutKwargs
      end
    end

    module WithoutKwargs
      def execute(sql, *)
        ...

      def exec_query(sql, *)
        ...
    end

    module WithKwargs
      def execute(sql, *)
        ...

      def exec_query(sql, *, **)
        ...

DatabaseRewinder::InsertRecorder.patch ::ActiveRecord::ConnectionAdapters::SQLite3Adapter if defined? ::ActiveRecord::ConnectionAdapters::SQLite3Adapter
...

[kyohsuke]

@amatsuda

Thank you for teaching another possible approach. I like the hardcoding approach because it makes the code shorter.

[amatsuda]

@kyohsuke Thank you!!