-
-
Notifications
You must be signed in to change notification settings - Fork 234
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Replace Config::Options#delegate to simple method definitions #185
Replace Config::Options#delegate to simple method definitions #185
Conversation
delegate method is required ActiveSupport. So it would raise NoMethodError with running in not rails environment.
Thanks for the fix. Could you please add a test for it? |
Well, we need to add not-rails environment to test it. I would try it by Sinatra. |
Cool, thanks! That should be easily possible with Appraisals |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ready to test with Sinatra! I confirmed to fail sinatra test with Config::Options#key?
and #has_key?
provided by ActiveSupport's #delegate
.
spec_helper.rb is updated to support sinatra, but CodeClimate tells an issue...
spec/spec_helper.rb
Outdated
|
||
def fixture_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For preventing to fail current specs, I defined fixture_path
manually in sinatra test. fixture_path
is provided by rspec-rails.
Yay, checks passed! Adding sinatra test was a bit harder than I thought 😅 |
You did a great job! :) I am waiting for Travis to report the build status and we can merge it in :) |
All good! Merging in and releasing... |
Fix #184.
Config::Options#delegate
is using ActiveSupport's#delegate
implicitly. So it would raiseNoMethodError
with running in not rails environment. e.g. Padrino, Sinatra and pure Ruby.It could fix it by replacing to simple method definitions.