geokit / geokit-rails

Official Geokit plugin for Rails/ActiveRecord. Provides location-based goodness for your Rails app. Requires the Geokit gem.
MIT License
1.57k stars 245 forks source link

Update *_distance_sql to work with customized latitude and longitude attribute names #72

Closed jensb closed 9 years ago

jensb commented 9 years ago

... as well as lat/lng attributes.

This has bugged me since Rails 3.0, please give it a look. Without this patch, it is not possible to use the distance_sql methods if you use custom lat/lng attribute names.

Thank you!

mnoack commented 9 years ago

Thanks @jensb for the PR. Overall it's good, but can you please address most of the comments houndci has raised even though some of them were problems before you started. You may ignore line lengths if you think they are overzealous as they often are. Also note that "methos" is missing the "d". e.g. fix the tabs/self

You can ignore the fact the build fails because this is due to the latest i18n not working with 1.8.7 and 1.9.2 and I'm happy to drop support for this once this code is merged.

mnoack commented 9 years ago

Also maybe you can deal with the long lines by extracting out this code in the origin object (just an example, you probably could do this better):

def get_lat(origin)
  origin.respond_to?(:lat) ? origin.lat : origin.send(:"#{lat_column_name}"
end
def get_lng(origin)
  origin.respond_to?(:lng) ? origin.lng : origin.send(:"#{lng_column_name}"
end

Then those other lines are simplified and shortened/hiding the fact that columns can be customised.

mnoack commented 9 years ago

And finally, to the extent that you can write a test for custom columns that would be great, to highlight the problem case you had.

jensb commented 9 years ago

Did as you requested. But could you please see if you can write a test? I looked at your test setup and it would probably take me a lot of time to write it in a way so that it integrates properly with your fixtures and mocks. In my code I just use an ActiveRecord object that has

acts_as_mappable :lat_column_name => :latitude, :lng_column_name => :longitude

Without my patches, things like

Foo.distance_sql(@bar).to_s

when Foo and Bar both have the above acts_as_mappable configuration would fail.

Thank you :)