apneadiving / Google-Maps-for-Rails

Enables easy Google map + overlays creation in Ruby apps
https://apneadiving.github.io/
MIT License
2.26k stars 382 forks source link

Why 'include InstanceMethods' in acts_as_gmappable? #120

Closed niuage closed 12 years ago

niuage commented 12 years ago

Hey,

I just want to understand something, since I'm really not an expert in these kind of things.

Why do you include InstanceMethods in acts_as_gmappable in the ActsAsGmappable module? I thought they were already included since ActsAsGmappable extend extend ActiveSupport::Concern. Is there a difference?

I was looking at this part of the code because I was wondering if it was possible to call acts_as_gmappable several times for one model. For instance, I'd like to do:

module Geolocated
    extend ActiveSupport::Concern

    included do
        acts_as_gmappable :check_process => true, :checker => "prevent_geocoding", :address => "location"
    end
end

class User < ActiveRecord::Base
    include Geolocated
    acts_as_gmappable :validation => false
end

Thanks :)

apneadiving commented 12 years ago

You are right, this was not useful (very good explanation here).

BTW, I removed it because o some deprecations in Rails 3.2.

Why did you need to add acts_as_gmappable several times to a model?

niuage commented 12 years ago

I want to call acts_as_gmappable several times because I have several models that are geolocated and share common code. So I extracted everything in a Geolocated module. But not every models have the same options. So I though it would be cool to be able to call acts_as_gmappable several times (each time you would call it, it would just merge the new hash to the already existing one, or the default).

I dont know if you ever used sunspot, but when you want to define a model as seachable, you do something like (in the model):

searchable do
    integer :id
    text :location
    text :description
    # ...
end

What's cool is that you can call searchable several times to describe what attributes to index, and so it's easy to extract this into modules.

Currently, I have to call acts_as_gmappable in every model.

niuage commented 12 years ago

https://github.com/rspec/rspec-rails/issues/474

Is this what you were talking about?

If I understood correctly, we now need to define our instance methods directly inside: included do; end...

apneadiving commented 12 years ago

It's the same issue yes. And I did as you suggest, I didn't fig further though.

apneadiving commented 12 years ago

Well understood for your code factoring. I didn't need to do that for maps but it's definitely an interesting point.

I've released a new version yesterday. Does it do the trick?

niuage commented 12 years ago

Actually, forgot what I said, I think that instance methods should just be defined in the module. It makes sense since we include the module, so instance methods are included by default. I don't really know why they chose to have an InstanceMethods module in the first place. Maybe for consistency, or maybe there was other advantages...