Refactor: Support for SQL Server (WIP for discussion)

[doconnor-clintel]

Previously:

• Add support to MSSQL/TSQL (Azure) #934
• SqlServer - ATAN2 bearing calculation fails #532
• https://gist.github.com/ttilberg/fe5ac60351ad33b865550c08857f612f via @ttilberg

Problem

Right now, SQL Server has bespoke SQL required to do bearing calculations.
Other datastores have a similar problem, ie SQLite, depending on what is available.

At some point, Geocoder::Sql was split out to try to make managing this a little bit easier, but it accidentally lead to the situation where:

• Geocoder::Store::Activerecord had a number of methods and conditionals to decide which of the other class' methods to call
• Geocoder::Sql doesn't know what database it is connected to, so can't generate correct SQL for every situation

Recommended solution

Ultimately, I think the responsibility for which datastore am I connected to does live in Geocoder::Store::Activerecord; but specific backends should be available for standard sql, vs sqlite, vs postgres, vs sql server.

This might seem like it violates DRY, but ultimately it would help acheive a single-responsibility-principle.

Transitioning and backwards compatibility

What I have done in this PR is really a step 1; where the standalone methods are marked deprecated.
This allows a future major version to remove them.

Additionally, at least for now I have shifted the sql generating concerns back into Geocoder::Store::Activerecord. This is because there is an implicit coupling between "which SQL should I generate" and "what am I connected to?". It introduces an extra layer of conditionals in the sql generation temporarily.

A subsequent PR should (with a major version to indicate the BC break)

• Implement Geocoder::Store::ActiveRecord::Postgres
• Implement Geocoder::Store::ActiveRecord::SQLServer
• Implement Geocoder::Store::ActiveRecord::SQLite
• Implement Geocoder::Store::ActiveRecord::SQLiteWithExtensions
• Implement Geocoder::Store::ActiveRecord::Postgis?
• Implement Geocoder::Store::ActiveRecord::OGC or SQL/MM if relevant?

What next?

If this kind of plan seems acceptable, I'm happy to help implement, test, refactor, etc.

If the feeling is still it doesn't fit; may fork (would want 1-2 other maintainers).

[alexreisner]

@doconnor-clintel Thanks for this and sorry for the delay. Of course you're right. The current "design" comes from many years of doing things quickly and simply. So I'm open to refactoring. This is a big project. If you're willing to take the lead, let's get started.

One thing to consider is that it's actually not necessary to deprecate anything. Methods in Geocoder::Sql could call methods in Geocoder::Store::ActiveRecord. Could we implement it that way first, and then deprecate the methods later? That will make it easier to release.

PS: ATN2. Wow.