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

Be more specific about the lookup of an Ollie::Base subclass. #2

Merged
merged 1 commit into from
Jul 5, 2013
Merged

Be more specific about the lookup of an Ollie::Base subclass. #2

merged 1 commit into from
Jul 5, 2013

Conversation

parkr
Copy link
Contributor

@parkr parkr commented Jun 27, 2013

On line 27 of checker.rb, Ollie does a lookup of the class within the Ollie namespace to instantiate the checker for the key given. It fails in cases where the new class isn't loaded quickly enough to be chosen by the call to .const_get.

To illustrate this, consider a Rails app using Redis. We're interested in fetching some info, so we write an Ollie::Redis class which

The Redis class is defined on the main Object namespace. If the lookup occurs before the Ollie::Redis class I've defined is loaded, then I Ollie tries to instantiate a new Redis object, because .const_get is called before Ollie::Redis is loaded.

This fix forces the key to be loaded from the Ollie namespace as the README prescribes.

@@ -24,7 +24,7 @@ def check_instances

def class_for_string(klass_string)
class_name = klass_string.to_s.camelize
Object.const_get("Ollie").const_get(class_name)
"Ollie::#{class_name}".constantize
Copy link
Member

Choose a reason for hiding this comment

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

I believe this change introduces a dependency on ActiveSupport to get String#constantize. If there's a way for you to get desired behavior using plain ol' Ruby, I'd be all 👍. Thank you for contributing!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely right, sorry about that!

What do you think of this:

Ollie.const_get(class_name, false)

The false tells Module not to inherit the constants from Ollie's superclass, so this should also have the desired behaviour.

Copy link
Member

Choose a reason for hiding this comment

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

If that gives you the desired behavior and @nixterrimus likes it, then 👍 😸

nixterrimus added a commit that referenced this pull request Jul 5, 2013
Be more specific about the lookup of an Ollie::Base subclass.
@nixterrimus nixterrimus merged commit ce0457e into modcloth-labs:master Jul 5, 2013
@nixterrimus
Copy link
Contributor

Thanks @parkr for the contribution!

Thanks @meatballhat for taking a look.

Looks good, merged in.

@parkr parkr deleted the specific-lookup branch July 5, 2013 16:30
@parkr
Copy link
Contributor Author

parkr commented Jul 5, 2013

Thanks, @nixterrimus! We've built a library around this (a gem with various checkers for common services) and it's working splendidly so far. Thanks for writing it!

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.

3 participants