Skip to content

Conversation

@TvL2386
Copy link

@TvL2386 TvL2386 commented May 18, 2020

Hi kklimuk,

I hope you like this.
This adds support for methods that have no arguments.

Kind regards,
TvL2386

@krnjn
Copy link
Collaborator

krnjn commented May 19, 2020

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)>'

@TvL2386
Copy link
Author

TvL2386 commented May 20, 2020

Hey @krnjn -- Would be great if you could show some code to reproduce the issue. And what rails version does work with that code?

@krnjn
Copy link
Collaborator

krnjn commented May 20, 2020

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

@TvL2386
Copy link
Author

TvL2386 commented May 20, 2020

Hi @krnjn -- it fails on 6.0.3 as well on my laptop, although it should work right? :)

$ bundle exec rails console
Running via Spring preloader in process 6440
Loading development environment (Rails 6.0.3)
irb(main):001:0> TestClass.new.get_date_interval_format('hour')
Traceback (most recent call last):
        2: from (irb):1
        1: from app/models/test_class.rb:6:in `get_date_interval_format'
ArgumentError (wrong number of arguments (given 2, expected 1))
irb(main):002:0> 

@TvL2386
Copy link
Author

TvL2386 commented May 20, 2020

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)

@krnjn
Copy link
Collaborator

krnjn commented May 20, 2020

Hm @TvL2386 -- does this work for you locally if you revert the changes made here: #2?

@TvL2386
Copy link
Author

TvL2386 commented May 20, 2020

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...

@TvL2386
Copy link
Author

TvL2386 commented May 20, 2020

Yeah related to ruby version. 2.6.6 does not work, 2.7.1 works

@TvL2386
Copy link
Author

TvL2386 commented May 20, 2020

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.
So it is given 2 arguments

@TvL2386
Copy link
Author

TvL2386 commented May 20, 2020

@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'
end

Not really sure whether the fix is awesome or aweful, but it works and demonstrates where the problem lies

@TvL2386
Copy link
Author

TvL2386 commented May 20, 2020

Sorry for the spam here. It seems I've been a bit too naive.
@kklimuk has a nice testsuite and the suite seems to run fine with ruby 2.6.6 and 2.7.1.
However, in reality there are errors.

@krnjn
Copy link
Collaborator

krnjn commented May 21, 2020

@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.

@kklimuk
Copy link
Owner

kklimuk commented May 21, 2020

hey folks! saw this PR and the associated issue.
i'm gonna take a bigger look later in the week and see if we can have a solution that works with both versions of ruby.
thanks for the report and the PR =)

@georgebancila
Copy link

@kklimuk can you take a look at this?

@beniaminmuresan
Copy link

hey folks! saw this PR and the associated issue.
i'm gonna take a bigger look later in the week and see if we can have a solution that works with both versions of ruby.
thanks for the report and the PR =)

Hi @kklimuk, do you think you can take a look at this one? Thanks 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants