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

Use a BasicObject with method_missing for the builder API #26

Merged
merged 8 commits into from
Jul 19, 2016

Conversation

timriley
Copy link
Member

Using define_singleton_method was bad because interferes with Ruby’s method caching.

I want to call out how I used respond_to? here. Because Builder is a BasicObject subclass now, and because BasicObject does not even implement #respond_to?, I just implemented that directly and therefore didn't have to bother with #respond_to_missing?.

Using define_singleton_method was bad because interferes with Ruby’s method caching.
return super unless strategies.key?(name)

Injector.new(container, strategies[name])
end
Copy link
Member

@solnic solnic Jul 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is minor stuff but a simple if/else is always easier to parse with eyes than unless with return so:

if strategies.key?(name)
  Injector.new(container, strategies[name])
else
  super
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh and method_missing should be private

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you're right @solnic, that if/else is much clearer for a method with only 2 single-line pathways :)

@solnic
Copy link
Member

solnic commented Jul 12, 2016

It would be nice to actually have specs for method_missing behavior, it's always so easy to mess it up by an accident and libs with messed up method missing behavior may cause weird issues :)

@timriley timriley merged commit 133eb92 into master Jul 19, 2016
@timriley timriley deleted the basic-object-builder-dsl branch July 19, 2016 07:49
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