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

Don't automatically load snail helpers (was: Don't include SnailHelpers if using Carmen-Rails gem) #19

Merged
merged 2 commits into from
Jun 28, 2013

Conversation

tinynumbers
Copy link
Contributor

Snail's implementation of region_options_for_select conflicts with a much more useful implementation in the widely-used carmen-rails gem. This change will check first to make sure the Carmen::Rails module is not defined before including the Snail helper that defines the conflicting version of region_options_for_select.

P.S. - I'm not really sure why Snail needs to implement region_options_for_select in the first place. It seems outside of the scope of the Snail gem, which is for pretty-formatting addresses, not for editing them. My preferred fix would be to just eliminate Snail's region_options_for_select, but perhaps somebody out there is using it...

…se Snail's region_options_for_select will conflict with the much more useful implementation of this helper method defined in Carmen::Rails.
@cainlevy
Copy link
Owner

Thanks! Snail used to supply more view helpers, but as you say, Carmen is a better source of geographic information. I think this helper and the Snail::REGIONS constant are leftovers.

Can we rely on Carmen being loaded first though?

@tinynumbers
Copy link
Contributor Author

Can we rely on Carmen being loaded first though?

Honestly, I don't know. In my case, it seems that carmen-rails always is loaded before snail, but I'm not sure how Bundler decided on the gem loading order. Alphabetical? Dunno.

However, if for some reason snail was being loaded before carmen-rails, a work-around would be to explicitly require carmen-rails before loading the bundler groups in application.rb. E.g.:

require 'active_record/railtie'
require 'action_controller/railtie'
require 'action_mailer/railtie'
require 'active_resource/railtie'
require 'sprockets/railtie'

require 'carmen-rails'

if defined?(Bundler)
  Bundler.require(*Rails.groups(assets: %w(development test)))
end

However, that's not really a good solution.

Maybe the best approach is just to delete the SnailHelpers?

@cainlevy
Copy link
Owner

How about a halfway step for the moment: we could leave the file in place but force people to require it themselves if they still need it, with a deprecation warning if they do? Might be nice to bundle Snail::REGIONS into the same file at that point, since they'll disappear together.

…nition in this file.

* Disabled automatic loading of Snail::Helpers and instead added Snail.load_helpers method
* Added deprecation warning when loading Snail::Helpers
@tinynumbers
Copy link
Contributor Author

Committed changes to implement what you suggested, @cainlevy - let me know if that works for you. Works for me.

@cainlevy
Copy link
Owner

Looks good!

cainlevy added a commit that referenced this pull request Jun 28, 2013
Don't automatically load snail helpers
@cainlevy cainlevy merged commit 44f2af1 into cainlevy:master Jun 28, 2013
@cainlevy
Copy link
Owner

released as snail-1.1.0

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.

None yet

2 participants