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 respond_to blowing up because of extra include_all argument #76

Merged
merged 1 commit into from Jul 1, 2014
Merged

fix respond_to blowing up because of extra include_all argument #76

merged 1 commit into from Jul 1, 2014

Conversation

grosser
Copy link
Collaborator

@grosser grosser commented Jun 26, 2014

calling respond_to?(:xxx, true) on Arturo blows up, instead of fixing it, how about we just kill that feature :)

@jamesarosen

@jamesarosen
Copy link
Contributor

Blows up how?

Removing the feature means bumping the major version, which we can do.

@grosser
Copy link
Collaborator Author

grosser commented Jun 26, 2014

ArgumentError since it's called with 2 instead of 1
I'd rather get rid of this feature / have less magic, not sure if you like
it / want to keep it.

On Thu, Jun 26, 2014 at 3:34 PM, James Alexander Rosen <
notifications@github.com> wrote:

Blows up how?

Removing the feature means bumping the major version, which we can do.


Reply to this email directly or view it on GitHub
#76 (comment).

@jamesarosen
Copy link
Contributor

Isn't it as easy as changing it to the following?

def respond_to?(symbol, include_private = false)
  symbol.to_s =~ ENABLED_FOR_METHOD_NAME || super
end

@grosser
Copy link
Collaborator Author

grosser commented Jun 26, 2014

yes, but I did not like that feature, so I was thinking, why not kill it ?
:)

On Thu, Jun 26, 2014 at 3:54 PM, James Alexander Rosen <
notifications@github.com> wrote:

Isn't it as easy as changing it to the following?

def respond_to?(symbol, include_private = false)
symbol.to_s =~ ENABLED_FOR_METHOD_NAME || superend


Reply to this email directly or view it on GitHub
#76 (comment).

@jamesarosen
Copy link
Contributor

I don't have any particular love for the feature. We should deprecate it first, though.

@grosser
Copy link
Collaborator Author

grosser commented Jun 27, 2014

this should be ready to merge too

@grosser
Copy link
Collaborator Author

grosser commented Jun 30, 2014

release, then merge this + release major bump ?

@jamesarosen
Copy link
Contributor

I'm traveling today. I can do releases and merges tomorrow.

jamesarosen added a commit that referenced this pull request Jul 1, 2014
fix respond_to blowing up because of extra include_all argument
@jamesarosen jamesarosen merged commit f7800d0 into zendesk:master Jul 1, 2014
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.

None yet

2 participants