Style and docs

Implement rubocop (with a couple rules turned off for easier migration) and document more methods.

.rubocop.yml

@@ -0,0 +1,32 @@
+AllCops:
+  DisplayCopNames: true
+  DisplayStyleGuide: true
+  TargetRubyVersion: 2.0
+  Exclude:
+  - 'vendor/**/*'
+
+Metrics/AbcSize:
+  Enabled: false
+
+Metrics/ClassLength:
+  Enabled: false
+
+Metrics/CyclomaticComplexity:
+  Enabled: false
+
+Metrics/MethodLength:
+  Enabled: false
+
+Metrics/LineLength:
+  Enabled: false
+
+Metrics/PerceivedComplexity:
+  Enabled: false
+
+Style/Documentation:
+  Enabled: false
+
+Style/PercentLiteralDelimiters:
+  PreferredDelimiters:
+    '%w': '[]'
+    '%i': '[]'

Rakefile

@@ -1,10 +1,14 @@
 require 'bundler/gem_tasks'
 require 'rake/testtask'
+require 'rubocop/rake_task'
 
 Rake::TestTask.new(:test) do |t|
   t.libs << 'test'
   t.libs << 'lib'
   t.test_files = FileList['test/**/*_test.rb']
 end
 
+RuboCop::RakeTask.new(:rubocop)
+Rake::Task[:test].prerequisites << :rubocop
+
 task default: :test

lib/active_record/connection_adapters/odbc_adapter.rb

@@ -18,6 +18,7 @@
 module ActiveRecord
   class Base
     class << self
+      # Build a new ODBC connection with the given configuration.
       def odbc_connection(config)
         config = config.symbolize_keys
 
@@ -27,7 +28,7 @@ def odbc_connection(config)
           elsif config.key?(:conn_str)
             odbc_conn_str_connection(config)
           else
-            raise ArgumentError, "No data source name (:dsn) or connection string (:conn_str) specified."
+            raise ArgumentError, 'No data source name (:dsn) or connection string (:conn_str) specified.'
           end
 
         database_metadata = ::ODBCAdapter::DatabaseMetadata.new(connection)
@@ -36,6 +37,7 @@ def odbc_connection(config)
 
       private
 
+      # Connect using a predefined DSN.
       def odbc_dsn_connection(config)
         username   = config[:username] ? config[:username].to_s : nil
         password   = config[:password] ? config[:password].to_s : nil
@@ -48,16 +50,9 @@ def odbc_dsn_connection(config)
       # e.g. "DSN=virt5;UID=rails;PWD=rails"
       #      "DRIVER={OpenLink Virtuoso};HOST=carlmbp;UID=rails;PWD=rails"
       def odbc_conn_str_connection(config)
-        connstr_keyval_pairs = config[:conn_str].split(';')
-
         driver = ODBC::Driver.new
         driver.name = 'odbc'
-        driver.attrs = {}
-
-        connstr_keyval_pairs.each do |pair|
-          keyval = pair.split('=')
-          driver.attrs[keyval[0]] = keyval[1] if keyval.length == 2
-        end
+        driver.attrs = config[:conn_str].split(';').map { |option| option.split('=', 2) }.to_h
 
         connection = ODBC::Database.new.drvconnect(driver)
         [connection, config.merge(driver: driver)]
@@ -75,34 +70,35 @@ class ODBCAdapter < AbstractAdapter
       ADAPTER_NAME = 'ODBC'.freeze
       BOOLEAN_TYPE = 'BOOLEAN'.freeze
 
-      ERR_DUPLICATE_KEY_VALUE     = 23505
-      ERR_QUERY_TIMED_OUT         = 57014
+      ERR_DUPLICATE_KEY_VALUE     = 23_505
+      ERR_QUERY_TIMED_OUT         = 57_014
       ERR_QUERY_TIMED_OUT_MESSAGE = /Query has timed out/
 
+      # The object that stores the information that is fetched from the DBMS
+      # when a connection is first established.
       attr_reader :database_metadata
 
       def initialize(connection, logger, config, database_metadata)
         super(connection, logger, config)
         @database_metadata = database_metadata
       end
 
-      # Returns the human-readable name of the adapter. Use mixed case - one
-      # can always use downcase if needed.
+      # Returns the human-readable name of the adapter.
       def adapter_name
         ADAPTER_NAME
       end
 
-      # Does this adapter support migrations? Backend specific, as the
-      # abstract adapter always returns +false+.
+      # Does this adapter support migrations? Backend specific, as the abstract
+      # adapter always returns +false+.
       def supports_migrations?
         true
       end
 
       # CONNECTION MANAGEMENT ====================================
 
-      # Checks whether the connection to the database is still active. This includes
-      # checking whether the database is actually capable of responding, i.e. whether
-      # the connection isn't stale.
+      # Checks whether the connection to the database is still active. This
+      # includes checking whether the database is actually capable of
+      # responding, i.e. whether the connection isn't stale.
       def active?
         @connection.connected?
       end
@@ -119,20 +115,24 @@ def reconnect!
           end
         super
       end
-      alias :reset! :reconnect!
+      alias reset! reconnect!
 
       # Disconnects from the database if already connected. Otherwise, this
       # method does nothing.
       def disconnect!
         @connection.disconnect if @connection.connected?
       end
 
+      # Build a new column object from the given options. Effectively the same
+      # as super except that it also passes in the native type.
+      # rubocop:disable Metrics/ParameterLists
       def new_column(name, default, sql_type_metadata, null, table_name, default_function = nil, collation = nil, native_type = nil)
         ::ODBCAdapter::Column.new(name, default, sql_type_metadata, null, table_name, default_function, collation, native_type)
       end
 
       protected
 
+      # Build the type map for ActiveRecord
       def initialize_type_map(map)
         map.register_type 'boolean',              Type::Boolean.new
         map.register_type ODBC::SQL_CHAR,         Type::String.new
@@ -165,13 +165,14 @@ def initialize_type_map(map)
         alias_type map, ODBC::SQL_TYPE_TIMESTAMP, ODBC::SQL_TIMESTAMP
       end
 
+      # Translate an exception from the native DBMS to something usable by
+      # ActiveRecord.
       def translate_exception(exception, message)
         error_number = exception.message[/^\d+/].to_i
 
-        case
-        when error_number == ERR_DUPLICATE_KEY_VALUE
+        if error_number == ERR_DUPLICATE_KEY_VALUE
           ActiveRecord::RecordNotUnique.new(message, exception)
-        when error_number == ERR_QUERY_TIMED_OUT, exception.message =~ ERR_QUERY_TIMED_OUT_MESSAGE
+        elsif error_number == ERR_QUERY_TIMED_OUT || exception.message =~ ERR_QUERY_TIMED_OUT_MESSAGE
           ::ODBCAdapter::QueryTimeoutError.new(message, exception)
         else
           super
@@ -180,6 +181,9 @@ def translate_exception(exception, message)
 
       private
 
+      # Can't use the built-in ActiveRecord map#alias_type because it doesn't
+      # work with non-string keys, and in our case the keys are (almost) all
+      # numeric
       def alias_type(map, new_type, old_type)
         map.register_type(new_type) do |_, *args|
           map.lookup(old_type, *args)

lib/odbc_adapter/adapters/mysql_odbc_adapter.rb

@@ -16,7 +16,7 @@ def arel_visitor
       # Explicitly turning off prepared statements in the MySQL adapter because
       # of a weird bug with SQLDescribeParam returning a string type for LIMIT
       # parameters. This is blocking them from running with an error:
-      # 
+      #
       #     You have an error in your SQL syntax; ...
       #     ... right syntax to use near ''1'' at line 1: ...
       def prepared_statements
@@ -49,11 +49,11 @@ def unquoted_false
         0
       end
 
-      def disable_referential_integrity(&block)
-        old = select_value("SELECT @@FOREIGN_KEY_CHECKS")
+      def disable_referential_integrity(&_block)
+        old = select_value('SELECT @@FOREIGN_KEY_CHECKS')
 
         begin
-          update("SET FOREIGN_KEY_CHECKS = 0")
+          update('SET FOREIGN_KEY_CHECKS = 0')
           yield
         ensure
           update("SET FOREIGN_KEY_CHECKS = #{old}")

lib/odbc_adapter/adapters/postgresql_odbc_adapter.rb

@@ -6,7 +6,7 @@ class PostgreSQLODBCAdapter < ActiveRecord::ConnectionAdapters::ODBCAdapter
       BOOLEAN_TYPE = 'bool'.freeze
       PRIMARY_KEY  = 'SERIAL PRIMARY KEY'.freeze
 
-      alias :create :insert
+      alias create insert
 
       # Override to handle booleans appropriately
       def native_database_types
@@ -35,7 +35,7 @@ def default_sequence_name(table_name, pk = nil)
         "#{table_name}_#{pk || 'id'}_seq"
       end
 
-      def sql_for_insert(sql, pk, id_value, sequence_name, binds)
+      def sql_for_insert(sql, pk, _id_value, _sequence_name, binds)
         unless pk
           table_ref = extract_table_ref_from_insert_sql(sql)
           pk = primary_key(table_ref) if table_ref
@@ -94,7 +94,7 @@ def create_database(name, options = {})
           when :connection_limit
             " CONNECTION LIMIT = #{value}"
           else
-            ""
+            ''
           end
         end
 
@@ -131,7 +131,7 @@ def remove_index!(_table_name, index_name)
         execute("DROP INDEX #{quote_table_name(index_name)}")
       end
 
-      def rename_index(table_name, old_name, new_name)
+      def rename_index(_table_name, old_name, new_name)
         execute("ALTER INDEX #{quote_column_name(old_name)} RENAME TO #{quote_table_name(new_name)}")
       end
 
@@ -149,8 +149,8 @@ def distinct(columns, orders)
         # Construct a clean list of column names from the ORDER BY clause,
         # removing any ASC/DESC modifiers
         order_columns = orders.map { |s| s.gsub(/\s+(ASC|DESC)\s*(NULLS\s+(FIRST|LAST)\s*)?/i, '') }
-        order_columns.reject! { |c| c.blank? }
-        order_columns = order_columns.zip((0...order_columns.size).to_a).map { |s,i| "#{s} AS alias_#{i}" }
+        order_columns.reject!(&:blank?)
+        order_columns = order_columns.zip((0...order_columns.size).to_a).map { |s, i| "#{s} AS alias_#{i}" }
 
         "DISTINCT #{columns}, #{order_columns * ', '}"
       end

lib/odbc_adapter/column.rb

@@ -2,6 +2,9 @@ module ODBCAdapter
   class Column < ActiveRecord::ConnectionAdapters::Column
     attr_reader :native_type
 
+    # Add the native_type accessor to allow the native DBMS to report back what
+    # it uses to represent the column internally.
+    # rubocop:disable Metrics/ParameterLists
     def initialize(name, default, sql_type_metadata = nil, null = true, table_name = nil, native_type = nil, default_function = nil, collation = nil)
       super(name, default, sql_type_metadata, null, table_name, default_function, collation)
       @native_type = native_type

lib/odbc_adapter/column_metadata.rb

@@ -13,7 +13,7 @@ class ColumnMetadata
       date:        [ODBC::SQL_TYPE_DATE, ODBC::SQL_DATE, ODBC::SQL_TYPE_TIMESTAMP, ODBC::SQL_TIMESTAMP],
       binary:      [ODBC::SQL_LONGVARBINARY, ODBC::SQL_VARBINARY],
       boolean:     [ODBC::SQL_BIT, ODBC::SQL_TINYINT, ODBC::SQL_SMALLINT, ODBC::SQL_INTEGER]
-    }
+    }.freeze
 
     attr_reader :adapter
 
@@ -56,7 +56,7 @@ def native_type_mapping(abstract, rows)
       create_params = selected_row[5]
       # Depending on the column type, the CREATE_PARAMS keywords can
       # include length, precision or scale.
-      if create_params && create_params.strip.length > 0 && abstract != :decimal
+      if create_params && !create_params.strip.empty? && abstract != :decimal
         result[:limit] = selected_row[2] # SQLGetTypeInfo: COL_SIZE
       end
 

lib/odbc_adapter/database_metadata.rb

@@ -11,7 +11,7 @@ class DatabaseMetadata
       SQL_MAX_TABLE_NAME_LEN
       SQL_USER_NAME
       SQL_DATABASE_NAME
-    ]
+    ].freeze
 
     attr_reader :values
 

lib/odbc_adapter/database_statements.rb

@@ -20,7 +20,7 @@ def execute(sql, name = nil, binds = [])
     # Executes +sql+ statement in the context of this connection using
     # +binds+ as the bind substitutes. +name+ is logged along with
     # the executed +sql+ statement.
-    def exec_query(sql, name = 'SQL', binds = [], prepare: false)
+    def exec_query(sql, name = 'SQL', binds = [], prepare: false) # rubocop:disable Lint/UnusedMethodArgument
       log(sql, name) do
         stmt =
           if prepared_statements
@@ -42,7 +42,7 @@ def exec_query(sql, name = 'SQL', binds = [], prepare: false)
 
         values = dbms_type_cast(columns.values, values)
         column_names = columns.keys.map { |key| format_case(key) }
-        result = ActiveRecord::Result.new(column_names, values)
+        ActiveRecord::Result.new(column_names, values)
       end
     end
 
@@ -52,7 +52,7 @@ def exec_query(sql, name = 'SQL', binds = [], prepare: false)
     def exec_delete(sql, name, binds)
       execute(sql, name, binds)
     end
-    alias :exec_update :exec_delete
+    alias exec_update exec_delete
 
     # Begins the transaction (and turns off auto-committing).
     def begin_db_transaction
@@ -81,7 +81,10 @@ def default_sequence_name(table, _column)
 
     private
 
-    def dbms_type_cast(columns, values)
+    # A custom hook to allow end users to overwrite the type casting before it
+    # is returned to ActiveRecord. Useful before a full adapter has made its way
+    # back into this repository.
+    def dbms_type_cast(_columns, values)
       values
     end
 
@@ -122,7 +125,7 @@ def native_case(identifier)
 
     # Assume column is nullable if nullable == SQL_NULLABLE_UNKNOWN
     def nullability(col_name, is_nullable, nullable)
-      not_nullable = (!is_nullable || nullable.to_s.match('NO') != nil)
+      not_nullable = (!is_nullable || !nullable.to_s.match('NO').nil?)
       result = !(not_nullable || nullable == SQL_NO_NULLS)
 
       # HACK!

lib/odbc_adapter/quoting.rb

@@ -18,7 +18,7 @@ def quote_column_name(name)
 
       # If upcase identifiers, only quote mixed case names.
       if database_metadata.upcase_identifiers?
-        return name unless (name =~ /([A-Z]+[a-z])|([a-z]+[A-Z])/)
+        return name unless name =~ /([A-Z]+[a-z])|([a-z]+[A-Z])/
       end
 
       "#{quote_char.chr}#{name}#{quote_char.chr}"
@@ -33,9 +33,9 @@ def quoted_date(value)
         if value.respond_to?(zone_conversion_method)
           value = value.send(zone_conversion_method)
         end
-        value.strftime("%Y-%m-%d %H:%M:%S") # Time, DateTime
+        value.strftime('%Y-%m-%d %H:%M:%S') # Time, DateTime
       else
-        value.strftime("%Y-%m-%d") # Date
+        value.strftime('%Y-%m-%d') # Date
       end
     end
   end