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

Use Arel instead of raw SQL #135

Closed flackou closed 6 years ago

flackou commented 6 years ago

I suggest to use Arel when it's possible, instead of raw SQL Strings. First, code is cleaner this way (in my point of view). Second, it offers new possibilities, like finding locations based on a max distance as a table's column, not a fixed number.

For example, imagine you want to find a Shop to deliver an Order at a specific location, but every Shop has its own max delivery distance (like some can deliver up to 20km, some others to 50km). You would do the query this way :

Shop.within(Shop.arel_table[:max_delivery_distance], origin: order)

I added a test for this use case.

I let the distance SQL formula as raw SQL cause it would be a mess to convert it as Arel ! Also I let the with_latlng as raw SQL because of a bug with Arel 6, used by Rails 4. And I kept the use of qualified_lat_column_name and qualified_lng_column_name even if I don't really understand this configuration option... (arel_table[:lat_column_name] would be better, as it would quote properly the table and column names depending on the DB, but perhaps some people override these configuration attributes ?).

I tested these changes on MySQL with Rails 3, 4 and 5.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.03%) to 84.397% when pulling bc8f89df5928a5aba1f20dfb0dda556e0f2da2a0 on flackou:arel into c370ad4649ab7f879e1d482aa9063c935f1a8923 on geokit:master.

mnoack commented 6 years ago

Thanks for the PR @flackou - It really cleans up that method and allows more possibilities.