-
Notifications
You must be signed in to change notification settings - Fork 2
added support for methods without arguments #4
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
base: master
Are you sure you want to change the base?
added support for methods without arguments #4
Conversation
|
Hey @kklimuk -- I'm noticing that for some reason with the Rails 6.0.3.1 upgrade, I'm getting this similar error with the number of arguments. I'm not sure what it's related to, but could you have a look and see if you are getting similar issues? ArgumentError:
wrong number of arguments (given 2, expected 1)
# ./app/services/time_series_utility.rb:82:in `get_date_interval_format'
# /Users/karannumero/.rvm/gems/ruby-2.6.6/gems/ruby_memoized-0.1.3/lib/ruby_memoized/memoizer.rb:12:in `call'
# /Users/karannumero/.rvm/gems/ruby-2.6.6/gems/ruby_memoized-0.1.3/lib/ruby_memoized.rb:37:in `block in method_added'
# ./app/services/accounts/contributions/time_series_reporter.rb:19:in `initialize'
# ./spec/services/accounts/contributions/time_series_reporter_spec.rb:8:in `new'
# ./spec/services/accounts/contributions/time_series_reporter_spec.rb:8:in `block (2 levels) in <top (required)>'
# ./spec/services/accounts/contributions/time_series_reporter_spec.rb:1042:in `block (5 levels) in <top (required)>' |
|
Hey @krnjn -- Would be great if you could show some code to reproduce the issue. And what rails version does work with that code? |
|
This is on Rails 6.0.3 and here's the code where this is now failing when I upgrade to Rails 6.0.3.1: class TestClass
include RubyMemoized
memoized
def get_date_interval_format(date_interval)
case date_interval
when 'hour'
'%l %p'
when 'day'
'%a'
when 'day_of_month'
'%b %-d'
when 'month'
'%B'
end
end |
|
Hi @krnjn -- it fails on 6.0.3 as well on my laptop, although it should work right? :) |
|
I don't think it has anything to do with rails. Outside of rails it fails as well: #!/bin/env ruby
require 'ruby_memoized'
class TestClass
include RubyMemoized
memoized
def get_date_interval_format(date_interval)
case date_interval
when 'hour'
'%l %p'
when 'day'
'%a'
when 'day_of_month'
'%b %-d'
when 'month'
'%B'
end
end
end
puts RUBY_VERSION
puts RubyMemoized::VERSION
TestClass.new.get_date_interval_format('hour')Output: $ app/models/test_class.rb
2.6.6
0.1.3
Traceback (most recent call last):
3: from app/models/test_class.rb:32:in `<main>'
2: from /home/tom/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/ruby_memoized-0.1.3/lib/ruby_memoized.rb:37:in `block in method_added'
1: from /home/tom/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/ruby_memoized-0.1.3/lib/ruby_memoized/memoizer.rb:12:in `call'
app/models/test_class.rb:15:in `get_date_interval_format': wrong number of arguments (given 2, expected 1) (ArgumentError) |
|
If I go back to v0.1.2 it works, but on v0.1.3 it does not. Not at all with a single argument. I don't even have methods I want to memoize with arguments, which is why I came here in the first place and it seems to just be broken. At least for my ruby 2.6.6. Looking into it... |
|
Yeah related to ruby version. 2.6.6 does not work, 2.7.1 works |
|
I think it's because this line in memoizer.rb: cache[[args, kwargs, block]] = context.send(method, *args, **kwargs, &block)calls the original version with the first argument and an empty hash for kwargs. |
|
@krnjn -- could you try this in your rails Gemfile: git 'https://github.com/TvL2386/ruby_memoized', branch: 'kwargs_fix_for_ruby26' do
gem 'ruby_memoized'
endNot really sure whether the fix is awesome or aweful, but it works and demonstrates where the problem lies |
|
Sorry for the spam here. It seems I've been a bit too naive. |
|
@TvL2386 I had to downgrade my ruby version anyways so will keep this locked as 0.1.2 until we upgrade, at which point I’ll just go to the next release. |
|
hey folks! saw this PR and the associated issue. |
|
@kklimuk can you take a look at this? |
Hi @kklimuk, do you think you can take a look at this one? Thanks 💯 |
Hi kklimuk,
I hope you like this.
This adds support for methods that have no arguments.
Kind regards,
TvL2386