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

Filters #38

Merged
merged 3 commits into from
Feb 3, 2015
Merged

Filters #38

merged 3 commits into from
Feb 3, 2015

Conversation

tommay
Copy link
Collaborator

@tommay tommay commented Jan 26, 2015

I was trying to figure out what compile! was good for. It converts a block to an UnboundMethod that can be bound to an Angelo::Base instance and called. But why bother. It seems more ruby to keep it as a block and call it with instance_eval, or instance_exec if it needs arguments.

This PR is strictly a refactoring with three somewhat related commits. Take them or leave them.

  • Filters are just blocks that are instance_evalled.
  • compile! is removed and everything else it was used for just stays as a block.
  • The filter invocation code is cleaned up.

Sorry to be kind of hyperactive. I've actually got any number of small tweaks to suggest that I've come across while going through the code.

@kenichi
Copy link
Owner

kenichi commented Jan 29, 2015

hmm, interesting. i certainly stole that directly from sinatra, i have a recollection that there was scope binding issues and UnboundMethod was The Way Around; but i can't remember exactly.

https://github.com/sinatra/sinatra/blob/master/lib/sinatra/base.rb#L1601

@tommay
Copy link
Collaborator Author

tommay commented Jan 30, 2015

I looked through the sinatra source and git history and my best guess for
why they use an UnboundMethod is because they need to run a block in their
object's context and they needed to pass parameters to it. instance_exec
does both of these things (instance_eval doesn't pass parameters) but I
believe it was added to ruby in 1.9 and that bit of sinatra was probably
coded for 1.8.

Unless/until there's a use case or a failing test that needs UnboundMethod,
I'm for keeping angelo simple and non-baffling. It certainly baffled me.
And if a time does come when UnboundMethod is required, we'll know why and
can drop in some comments.

On Thu, Jan 29, 2015 at 11:56 AM, kenichi nakamura <notifications@github.com

wrote:

hmm, interesting. i certainly stole that directly from sinatra, i have a
recollection that there was scope binding issues and UnboundMethod was The
Way Around; but i can't remember exactly.


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

@kenichi
Copy link
Owner

kenichi commented Feb 3, 2015

i agree. unboundmethod seems to be unnecessary black magic at this point.

kenichi added a commit that referenced this pull request Feb 3, 2015
@kenichi kenichi merged commit ede49f2 into kenichi:master Feb 3, 2015
@tommay tommay deleted the filters branch February 3, 2015 18:09
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