Check to see if adapter is loaded before requiring an explicit file #83
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -93,7 +93,9 @@ module ClassMethods | |||
| # A proxy to an instance of a finder adapter, inferred from the connection's adapter. | |||
| def adapter | |||
| @adapter ||= begin | |||
| require File.join('geokit-rails', 'adapters', connection.adapter_name.downcase) | |||
| unless Adapters.const_defined?(connection.adapter_name.camelcase) | |||
| require File.join('geokit-rails', 'adapters', connection.adapter_name.downcase) | |||
There was a problem hiding this comment.
Line is too long. [91/80]
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
| require File.join('geokit-rails', 'adapters', connection.adapter_name.downcase) | ||
| unless Adapters.const_defined?(connection.adapter_name.camelcase) | ||
| filename = connection.adapter_name.downcase | ||
| require File.join('geokit-rails', 'adapters', filename) |
There was a problem hiding this comment.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
|
Sorry for not merging this earlier. Excellent fix, although I hope it doesn't result in people not submitting their geocoders via PR's |
mnoack
pushed a commit
that referenced
this pull request
Aug 23, 2015
Check to see if adapter is loaded before requiring an explicit file
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change simply checks to see if the database-specific adapter is already loaded before it tries to require a file with an explicit path. With this change, projects that use geokit-rails can choose to provide their own adapters in initializers without causing unnecessary UnsupportedAdapter errors.
This simple change is the end of a very long story for my team. (Read on if you dare!) Our Rails app uses a custom database adapter to perform cryptographic functions (the excellent pgcrypto adapter by @flipsasser). For Geokit's purposes, the PGCrypto adapter can be treated identically to the PostgreSQL adapter. We had a lot of trouble getting geokit-rails to handle the situation correctly:
class Geokit::Adapters::PGCrypto < Geokit::Adapters::PostgreSQL; endbut the question was where to put that code./lib/geokit-rails/adapters/pgcrypto.rb. That allowed geokit-rails to work, but only in lazy-loaded environments. In eager-loaded environments, Rails would find that file before geokit-rails did, and it would raise a NameError because it expects a class likeGeokit::Adapters::PGCryptoto be found at the path/lib/geokit/adapters/pgcrypto.rbinstead./lib/geokit/adapters/pgcrypto.rb, where Rails would expect it to be. This allows eager-loading to work, but geokit-rails would still trip and fall over anyway because therequireon line 96 was explicitly looking for a file undergeokit-rails/./lib/geokit-rails/adapters/pgcrypto.rbthat simply loaded the file at/lib/geokit/adapters/pgcrypto.rb, but this suffered from the same problem where Rails would raise a NameError when trying to eager-load the file.After tearing our hair out for a while, we decided that the best solution was to fork the gem and submit the PR you see here. We think this is a pretty common-sense solution that might benefit others without requiring any other changes.