Include delegators only in Sinatra classes, not in Object #396

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@Veraticus

Sinatra::Delegators is getting mixed in to Object, which forces in Object#get, Object#before, etc. This messes up method_missing stuff and is just generally a bad idea. This patch seems to keep all tests passing and makes sure that the delegators are only included in the actual Sinatra classes.

@rkh

This comment has been minimized.

Show comment
Hide comment
@rkh

rkh Oct 31, 2011

Member

Hi there, thanks for the pull request!

I can't merge this in though. This would break most Sinatra applications. We intentionally include it in Object. Otherwise classic style applications won't work. Example:

require 'sinatra'

get '/' do
  "with your patch, it would raise a NameError"
end

This is why we do this when you require "sinatra" but not when you require "sinatra/base". See Modular vs. Classic Style. I will add a test that will fail with your patch.

The issue with method_missing only occurs with sloppily implemented delegation. If you define method_missing you should also define respond_to?, which Sinatra will honor.

Member

rkh commented Oct 31, 2011

Hi there, thanks for the pull request!

I can't merge this in though. This would break most Sinatra applications. We intentionally include it in Object. Otherwise classic style applications won't work. Example:

require 'sinatra'

get '/' do
  "with your patch, it would raise a NameError"
end

This is why we do this when you require "sinatra" but not when you require "sinatra/base". See Modular vs. Classic Style. I will add a test that will fail with your patch.

The issue with method_missing only occurs with sloppily implemented delegation. If you define method_missing you should also define respond_to?, which Sinatra will honor.

@rkh rkh closed this Oct 31, 2011

@Veraticus

This comment has been minimized.

Show comment
Hide comment
@Veraticus

Veraticus Oct 31, 2011

Thanks for the response Konstantin, that makes a lot of sense!

Thanks for the response Konstantin, that makes a lot of sense!

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