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

Fix fallthrough accessor method_missing not passing all options to super #364

Closed
wants to merge 2 commits into from

Conversation

doits
Copy link
Contributor

@doits doits commented Mar 11, 2020

When mobility handled something it wasn't responsible for, it didn't pass all options to super and actually swallowed options. Other method missing handlers couldn't work as intended then.

@doits doits changed the title Fix fallthrough acessor method missing not passing all options to super Fix fallthrough accessor method_missing not passing all options to super Mar 11, 2020
@doits
Copy link
Contributor Author

doits commented Mar 11, 2020

Oh and just one more idea (not yet fully tested and not thought to the end): It might be possible to speed up fallthrough accessor by defining an instance method inside method_missing, something like this:

        define_method :method_missing do |method_name, *arguments, **options, &block|
          if method_name =~ method_name_regex
            attribute = $1.to_sym
            locale, suffix = $2.split('_')
            locale = "#{locale}-#{suffix.upcase}" if suffix

            singleton_class.define_method(method_name) do |*arguments, **options|
              public_send("#{attribute}#{$4}", *arguments, **options, locale: locale.to_sym)
            end

            public_send(method_name, *arguments, **options)
          else
            super(method_name, *arguments, **options, &block)
          end
        end

This way method_missing will handle it only once and define the instance method. Next calls to this method will not go through method_missing anymore.

One has to think about how the method is defined exactly (especially the arguments), but this could be a way to speed up fallthrough accessors which will then have only a penalty on first call. Though this is something for another PR.

@doits
Copy link
Contributor Author

doits commented Mar 27, 2020

@shioyama maybe you can find some time to look at this? It is a fairly small change that unbreaks my App.

@shioyama
Copy link
Owner

shioyama commented Mar 28, 2020 via email

@shioyama
Copy link
Owner

Sorry for the delay! Thanks for the change, it looks great 👍 Do you think you could add a test so we avoid any future regression?

@shioyama
Copy link
Owner

shioyama commented May 14, 2020

Never mind I added a spec in abd67a2. If specs pass I'll merge & release 🙂

@doits
Copy link
Contributor Author

doits commented May 14, 2020

Thanks!

@shioyama
Copy link
Owner

It's passing 🎉 I'll merge it and deploy later tonight, thanks for catching this!

@shioyama
Copy link
Owner

shioyama commented May 15, 2020

This has been merged in 7b4ca04 (0-8-stable branch) and 4729c19 (master branch).

@shioyama shioyama closed this May 15, 2020
@shioyama
Copy link
Owner

Released in 0.8.12 🎉

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.

2 participants