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

Support kwargs for named scopes #39196

Closed
wants to merge 1 commit into from
Closed

Conversation

jhawthorn
Copy link
Member

Previously Ruby 2.7 would generate a warning when trying to define a named scope which took keyword arguments.

class Books
  scope :with_attributes, -> (author: nil, title: nil) {
    # ...
  }
end

This should allow those scopes to be defined and run without warnings.

cc @eileencodes @kytrinyx @HParker

@@ -407,9 +407,16 @@ def scoping
already_in_scope? ? yield : _scoping(self) { yield }
end

def _exec_scope(name, *args, &block) # :nodoc:
def _exec_scope(name, *args, **kwargs, &block) # :nodoc:
Copy link
Member

@kamipo kamipo May 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use singleton_class.send(:ruby2_keywords, name) if respond_to?(:ruby2_keywords, true)?

diff --git a/activerecord/lib/active_record/scoping/named.rb b/activerecord/lib/active_record/scoping/named.rb
index e2167881e5..8ffdadae54 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

Strictly speaking, *args to *args, **kwargs is not perfectly compatible except Ruby 3.0, ruby2_keywords is only way to do that as long as support Ruby 2.x.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like ruby2_keywords is only valid for methods taking kwargs or a splat.

named.rb:198: warning: Skipping set of ruby2_keywords flag for tagged_with (method accepts keywords or method does not accept argument splat)

I also wanted to avoid this because we should see warnings in 2.7 if a named scope takes kwargs but is passed a hash.

A agree that the *args, **kwargs doesn't quite work here. It might be that the only way to do this is using some if RUBY_VERSION >= checks and duplicating these two methods 😩

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added test case for generate_relation_method 7ce3451, it also demonstrates a regression for this PR.

Can you apply the above patch singleton_class.send(:ruby2_keywords, name) if respond_to?(:ruby2_keywords, true) in lib/active_record/scoping/named.rb instead of this PR? That patch is only way to pass 7ce3451 and address kwargs warnings in Ruby 2.7.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per my last comment we can't use that here as it causes even more warnings.

kamipo added a commit to kamipo/rails that referenced this pull request May 8, 2020
We fixed `generate_relation_method` to address kwargs warnings at
rails#38038, but I missed generated named scopes also need the same fix.

Test case has picked from rails#39196.

Co-authored-by: John Hawthorn <john@hawthorn.email>
@jhawthorn
Copy link
Member Author

Closed for #39199

@jhawthorn jhawthorn closed this May 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants