Merge pull request #1501 from lovro-bikic/rails-order-arguments

Add new cop `Rails/OrderArguments`

Author: koic

changelog/new_cop_rails_order_arguments.md

@@ -0,0 +1 @@
+* [#1501](https://github.com/rubocop/rubocop-rails/pull/1501): Add new cop `Rails/OrderArguments`. ([@lovro-bikic][])

config/default.yml

@@ -748,6 +748,13 @@ Rails/NotNullColumn:
   Include:
     - db/**/*.rb
 
+Rails/OrderArguments:
+  Description: 'Prefer symbol arguments over strings in `order` method.'
+  StyleGuide: 'https://rails.rubystyle.guide/#order-arguments'
+  Enabled: pending
+  VersionAdded: '<<next>>'
+  Safe: false
+
 Rails/OrderById:
   Description: >-
                   Do not use the `id` column for ordering.

lib/rubocop/cop/rails/order_arguments.rb

@@ -0,0 +1,79 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Rails
+      # Prefer symbol arguments over strings in `order` method.
+      #
+      # @safety
+      #   Cop is unsafe because the receiver might not be an Active Record query.
+      #
+      # @example
+      #   # bad
+      #   User.order('name')
+      #   User.order('name DESC')
+      #
+      #   # good
+      #   User.order(:name)
+      #   User.order(name: :desc)
+      #
+      class OrderArguments < Base
+        extend AutoCorrector
+
+        MSG = 'Prefer `%<prefer>s` instead.'
+
+        RESTRICT_ON_SEND = %i[order].freeze
+
+        def_node_matcher :string_order, <<~PATTERN
+          (call _ :order (str $_value)+)
+        PATTERN
+
+        ORDER_EXPRESSION_REGEX = /\A(\w+) ?(asc|desc)?\z/i.freeze
+
+        def on_send(node)
+          return unless (current_expressions = string_order(node))
+          return unless (preferred_expressions = replacement(current_expressions))
+
+          offense_range = find_offense_range(node)
+          add_offense(offense_range, message: format(MSG, prefer: preferred_expressions)) do |corrector|
+            corrector.replace(offense_range, preferred_expressions)
+          end
+        end
+        alias on_csend on_send
+
+        private
+
+        def find_offense_range(node)
+          node.first_argument.source_range.join(node.last_argument.source_range)
+        end
+
+        def replacement(order_expressions)
+          order_arguments = order_expressions.flat_map { |expr| expr.split(',') }
+          order_arguments.map! { |arg| extract_column_and_direction(arg.strip) }
+
+          return if order_arguments.any?(&:nil?)
+
+          convert_to_preferred_arguments(order_arguments).join(', ')
+        end
+
+        def convert_to_preferred_arguments(order_expressions)
+          use_hash = false
+          order_expressions.map do |column, direction|
+            if direction == :asc && !use_hash
+              ":#{column}"
+            else
+              use_hash = true
+              "#{column}: :#{direction}"
+            end
+          end
+        end
+
+        def extract_column_and_direction(order_expression)
+          return unless (column, direction = ORDER_EXPRESSION_REGEX.match(order_expression)&.captures)
+
+          [column.downcase, direction&.downcase&.to_sym || :asc]
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/rails_cops.rb

@@ -82,6 +82,7 @@
 require_relative 'rails/multiple_route_paths'
 require_relative 'rails/negate_include'
 require_relative 'rails/not_null_column'
+require_relative 'rails/order_arguments'
 require_relative 'rails/order_by_id'
 require_relative 'rails/output'
 require_relative 'rails/output_safety'

spec/rubocop/cop/rails/order_arguments_spec.rb

@@ -0,0 +1,131 @@
+# frozen_string_literal: true
+
+RSpec.describe RuboCop::Cop::Rails::OrderArguments, :config do
+  it 'registers an offense for `order` with a string argument' do
+    expect_offense(<<~RUBY)
+      User.order('first_name')
+                 ^^^^^^^^^^^^ Prefer `:first_name` instead.
+    RUBY
+
+    expect_correction(<<~RUBY)
+      User.order(:first_name)
+    RUBY
+  end
+
+  it 'registers an offense for `order` with multiple string arguments' do
+    expect_offense(<<~RUBY)
+      User.order('first_name', 'last_name')
+                 ^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `:first_name, :last_name` instead.
+    RUBY
+
+    expect_correction(<<~RUBY)
+      User.order(:first_name, :last_name)
+    RUBY
+  end
+
+  it 'registers an offense for `order` with a string argument with DESC direction' do
+    expect_offense(<<~RUBY)
+      User.order('name DESC')
+                 ^^^^^^^^^^^ Prefer `name: :desc` instead.
+    RUBY
+
+    expect_correction(<<~RUBY)
+      User.order(name: :desc)
+    RUBY
+  end
+
+  it 'registers an offense for `order` with a string argument with ASC order' do
+    expect_offense(<<~RUBY)
+      User.order('name ASC')
+                 ^^^^^^^^^^ Prefer `:name` instead.
+    RUBY
+
+    expect_correction(<<~RUBY)
+      User.order(:name)
+    RUBY
+  end
+
+  it 'registers an offense for `order` with DESC column followed by implicit ASC column' do
+    expect_offense(<<~RUBY)
+      User.order('first_name DESC, last_name')
+                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `first_name: :desc, last_name: :asc` instead.
+    RUBY
+
+    expect_correction(<<~RUBY)
+      User.order(first_name: :desc, last_name: :asc)
+    RUBY
+  end
+
+  it 'registers an offense for `order` with explicit ASC column followed by implicit ASC column' do
+    expect_offense(<<~RUBY)
+      User.order('first_name ASC, last_name')
+                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `:first_name, :last_name` instead.
+    RUBY
+
+    expect_correction(<<~RUBY)
+      User.order(:first_name, :last_name)
+    RUBY
+  end
+
+  it 'registers an offense for safe navigation `order` with a string argument' do
+    expect_offense(<<~RUBY)
+      User&.order('first_name')
+                  ^^^^^^^^^^^^ Prefer `:first_name` instead.
+    RUBY
+
+    expect_correction(<<~RUBY)
+      User&.order(:first_name)
+    RUBY
+  end
+
+  it 'registers an offense for `order` with multiple string arguments, one of which has multiple sorts' do
+    expect_offense(<<~RUBY)
+      User.order('first_name, middle_name', 'last_name')
+                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `:first_name, :middle_name, :last_name` instead.
+    RUBY
+
+    expect_correction(<<~RUBY)
+      User.order(:first_name, :middle_name, :last_name)
+    RUBY
+  end
+
+  it 'registers an offense for `order` with lowercase direction' do
+    expect_offense(<<~RUBY)
+      User.order('name desc')
+                 ^^^^^^^^^^^ Prefer `name: :desc` instead.
+    RUBY
+
+    expect_correction(<<~RUBY)
+      User.order(name: :desc)
+    RUBY
+  end
+
+  it 'registers an offense for `order` with uppercase column' do
+    expect_offense(<<~RUBY)
+      User.order('NAME DESC')
+                 ^^^^^^^^^^^ Prefer `name: :desc` instead.
+    RUBY
+
+    expect_correction(<<~RUBY)
+      User.order(name: :desc)
+    RUBY
+  end
+
+  it 'does not register an offense for `order` with symbol argument' do
+    expect_no_offenses(<<~RUBY)
+      User.order(:first_name)
+    RUBY
+  end
+
+  it 'does not register an offense for `order` with a string and a symbol argument' do
+    expect_no_offenses(<<~RUBY)
+      User.order(:first_name, 'last_name')
+    RUBY
+  end
+
+  it 'does not register an offense for `order` with string argument expression' do
+    expect_no_offenses(<<~RUBY)
+      User.order('LEFT(first_name, 1)')
+    RUBY
+  end
+end