Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix warnings #608

Merged
merged 6 commits into from Feb 24, 2014

Conversation

Projects
None yet
2 participants
Contributor

exviva commented Feb 23, 2014

Fix several warnings and one bug (using respond_to? instead of method_defined?)

Owner

alexreisner commented Feb 23, 2014

This is interesting, but we need to fix the test failures. (Why is the use of respond_to? a bug? It provides stronger protection against overwriting something useful than method_defined? which will not catch functionality implemented with, for example, method_missing.)

Contributor

exviva commented Feb 23, 2014

@alexreisner respond_to? will almost always return false in this scope (of the Test class, not an instance of Test). We want to check if an instance of Test responds to, say, location, and not the Test class object.

I'll look into the failed build now.

Contributor

exviva commented Feb 23, 2014

Test failures are actually exposing the bug I've mentioned - the method address was always being redefined because if respond_to?(:address) was always false.

The solution I came up with involves removing the method before redefining it - similar to how activesupport is doing it.

Owner

alexreisner commented Feb 24, 2014

OK, thanks for the fix. This is great! Makes me think we should work towards the elimination of all warnings.

alexreisner added a commit that referenced this pull request Feb 24, 2014

@alexreisner alexreisner merged commit 3c0859c into alexreisner:master Feb 24, 2014

1 check passed

default The Travis CI build passed
Details
Contributor

exviva commented Feb 24, 2014

Thanks for merging! I've run my project's specs with this version of geocoder, and these were the warnings that I've caught. Probably running geocoder's specs with -w would reveal even more warnings.

Owner

alexreisner commented Feb 24, 2014

Yep, plenty more are thrown by the test suite. Thanks for getting the process started!

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