rmm5t / strip_attributes

:hocho: An ActiveModel extension that automatically strips all attributes of leading and trailing whitespace before validation. If the attribute is blank, it strips the value to nil.
https://github.com/rmm5t/strip_attributes
MIT License
558 stars 51 forks source link

Fix Ruby deprecation warning #56

Closed corny closed 2 years ago

corny commented 4 years ago

I am getting:

ruby/2.7.0/gems/strip_attributes-1.11.0/lib/strip_attributes.rb:8: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
ruby/2.7.0/gems/activemodel-6.0.3.2/lib/active_model/callbacks.rb:130: warning: The called method is defined here
rmm5t commented 4 years ago

Hi. Thanks for this.

How did you get the deprecation warning to exhibit itself in the first place? I just ran the test suite against Ruby 2.7.1 and I did not get any deprecation warnings.

$ ruby --version
ruby 2.7.1p83 (2020-03-31 revision a0c7c23c9c) [x86_64-darwin19]

$ bundle exec rake
Started with run options --seed 44204

  41/41: [======================] 100% Time: 00:00:00, Time: 00:00:00

Finished in 0.04842s
41 tests, 157 assertions, 0 failures, 0 errors, 0 skips
corny commented 3 years ago

You probably have enable output of warnings. I've bumped the required_ruby_version to 2.0.

swrobel commented 3 years ago

I'm pretty sure this will cause an exception on Ruby 3.0. It might be time to merge this.

corny commented 3 years ago

Yes please!

rmm5t commented 2 years ago

I still cannot reproduce this warning. Even with RUBYOPT="-W2 -v" bundle exec rake. Can someone please demonstrate a command and ruby version combination to get these warnings to print?

swrobel commented 2 years ago

Yeah, this is weird, I genuinely have no idea why this isn't triggered in the test suite on Ruby 2.7, and why the suite passes just fine on Ruby 3. I have, however, seen this in a Rails app, but I don't have a good example in hand. I understand your desire to be able to repro, @rmm5t, but is there any objection to merging? This seems to be a well-documented syntax change for Ruby 3.

rmm5t commented 2 years ago

"This seems to be a well-documented syntax change for Ruby 3."

@swrobel I'm confused by your assertion. This only applies to keyword arguments. The method in question in this PR does not accept keyword arguments.

See: https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/

"Note that Ruby 3.0 doesn’t behave differently when calling a method which doesn’t accept keyword arguments with keyword arguments. For instance, the following case is not going to be deprecated and will keep working in Ruby 3.0. The keyword arguments are still treated as a positional Hash argument."

def foo(kwargs = {})
  kwargs
end

foo(k: 1) #=> {:k=>1}
rmm5t commented 2 years ago

Hi all. I'm going to close this PR as invalid.

  1. I don't understand how the code violates the original deprecation warning.
  2. I don't see how this proposed fix fixes everything even if the deprecation warning still exists, because the pattern is still used elsewhere in the same file.
  3. The line in question from the claimed deprecation warning comes from line 8, which is a call to before_validation (not strip_attributes). It's possible it's a line mismatch because of the block, but I'm not sure about that.
  4. It looks like the warning from this PR occurred with activemodel v6.0.3.2 and ruby 2.7.x.
  5. Using this PR's patch (against activemodel v6.0.3.2), I get more warnings against activemodel:
    /.../strip_attributes/lib/strip_attributes.rb:6: warning: Passing the keyword argument as the last hash parameter is deprecated
    /.../strip_attributes/lib/strip_attributes.rb:97: warning: The called method `validate_options' is defined here

I'm willing to reconsider changing strip_attributes, but only until someone can document a way to recreate the warning against master. I'll need a specific ruby version, activemodel version, etc. Right now, I do not see any problems and the codebase is green against Ruby 2.7, 3.0, and 3.1.