Skip to content

added support for methods without arguments#4

Open
TvL2386 wants to merge 1 commit into
kklimuk:masterfrom
TvL2386:support_for_methods_without_arguments
Open

added support for methods without arguments#4
TvL2386 wants to merge 1 commit into
kklimuk:masterfrom
TvL2386:support_for_methods_without_arguments

Conversation

@TvL2386

@TvL2386 TvL2386 commented May 18, 2020

Copy link
Copy Markdown

Hi kklimuk,

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

Kind regards,
TvL2386

@krnjn

krnjn commented May 19, 2020

Copy link
Copy Markdown
Collaborator

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

TvL2386 commented May 20, 2020

Copy link
Copy Markdown
Author

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

krnjn commented May 20, 2020

Copy link
Copy Markdown
Collaborator

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

TvL2386 commented May 20, 2020

Copy link
Copy Markdown
Author

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

TvL2386 commented May 20, 2020

Copy link
Copy Markdown
Author

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

krnjn commented May 20, 2020

Copy link
Copy Markdown
Collaborator

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

@TvL2386

TvL2386 commented May 20, 2020

Copy link
Copy Markdown
Author

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

TvL2386 commented May 20, 2020

Copy link
Copy Markdown
Author

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

@TvL2386

TvL2386 commented May 20, 2020

Copy link
Copy Markdown
Author

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

TvL2386 commented May 20, 2020

Copy link
Copy Markdown
Author

@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

TvL2386 commented May 20, 2020

Copy link
Copy Markdown
Author

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

krnjn commented May 21, 2020

Copy link
Copy Markdown
Collaborator

@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

kklimuk commented May 21, 2020

Copy link
Copy Markdown
Owner

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
Copy Markdown

@kklimuk can you take a look at this?

@beniaminmuresan

Copy link
Copy Markdown

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