Merge pull request #50384 from skipkayhil/hm-fix-alphabetical-configuring

Fix railspect not validating alphabetical order

guides/source/configuring.md

@@ -209,14 +209,6 @@ Makes application believe that all requests are arriving over SSL. This is usefu
 Enables writing log file output immediately instead of buffering. Defaults to
 `true`.
 
-#### `config.autoload_once_paths`
-
-Accepts an array of paths from which Rails will autoload constants that won't be wiped per request. Relevant if reloading is enabled, which it is by default in the `development` environment. Otherwise, all autoloading happens only once. All elements of this array must also be in `autoload_paths`. Default is an empty array.
-
-#### `config.autoload_paths`
-
-Accepts an array of paths from which Rails will autoload constants. Default is an empty array. Since [Rails 6](upgrading_ruby_on_rails.html#autoloading), it is not recommended to adjust this. See [Autoloading and Reloading Constants](autoloading_and_reloading_constants.html#autoload-paths).
-
 #### `config.autoload_lib(ignore:)`
 
 This method adds `lib` to `config.autoload_paths` and `config.eager_load_paths`.
@@ -235,6 +227,14 @@ The method `config.autoload_lib_once` is similar to `config.autoload_lib`, excep
 
 By calling `config.autoload_lib_once`, classes and modules in `lib` can be autoloaded, even from application initializers, but won't be reloaded.
 
+#### `config.autoload_once_paths`
+
+Accepts an array of paths from which Rails will autoload constants that won't be wiped per request. Relevant if reloading is enabled, which it is by default in the `development` environment. Otherwise, all autoloading happens only once. All elements of this array must also be in `autoload_paths`. Default is an empty array.
+
+#### `config.autoload_paths`
+
+Accepts an array of paths from which Rails will autoload constants. Default is an empty array. Since [Rails 6](upgrading_ruby_on_rails.html#autoloading), it is not recommended to adjust this. See [Autoloading and Reloading Constants](autoloading_and_reloading_constants.html#autoload-paths).
+
 #### `config.beginning_of_week`
 
 Sets the default beginning of week for the
@@ -310,10 +310,6 @@ Sets the format used in responses when errors occur in the development environme
 
 Controls whether or not someone can start a console in sandbox mode. This is helpful to avoid a long running session of sandbox console, that could lead a database server to run out of memory. Defaults to `false`.
 
-#### `config.sandbox_by_default`
-
-When `true`, rails console starts in sandbox mode. To start rails console in non-sandbox mode, `--no-sandbox` must be specified. This is helpful to avoid accidental writing to the production database. Defaults to `false`.
-
 #### `config.dom_testing_default_html_version`
 
 Controls whether an HTML4 parser or an HTML5 parser is used by default by the test helpers in Action View, Action Dispatch, and `rails-dom-testing`.
@@ -539,6 +535,10 @@ Enables or disables reloading of classes only when tracked files change. By defa
 
 Causes the app to not boot if a master key hasn't been made available through `ENV["RAILS_MASTER_KEY"]` or the `config/master.key` file.
 
+#### `config.sandbox_by_default`
+
+When `true`, rails console starts in sandbox mode. To start rails console in non-sandbox mode, `--no-sandbox` must be specified. This is helpful to avoid accidental writing to the production database. Defaults to `false`.
+
 #### `config.secret_key_base`
 
 The fallback for specifying the input secret for an application's key generator.

tools/rail_inspector/lib/rail_inspector/configuring/check/general_configuration.rb

@@ -6,17 +6,37 @@ module RailInspector
   class Configuring
     module Check
       class GeneralConfiguration
-        attr_reader :checker
+        class AccessorParser
+          def initialize(checker)
+            @checker = checker
+          end
 
-        def initialize(checker)
+          def call
+            visitor = Visitor::Attribute.new
+            visitor.visit(app_config_tree)
+            visitor.attribute_map[APP_CONFIG_CONST]["attr_accessor"]
+          end
+
+          private
+            APP_CONFIG_CONST = "Rails::Application::Configuration"
+
+            def app_config_tree
+              @checker.parse(APPLICATION_CONFIGURATION_PATH)
+            end
+        end
+
+        attr_reader :checker, :expected_accessors
+
+        def initialize(checker, expected_accessors: AccessorParser.new(checker).call)
           @checker = checker
+          @expected_accessors = expected_accessors
         end
 
         def check
           header, *config_sections = documented_general_config
 
           non_nested_accessors =
-            general_accessors.reject do |a|
+            expected_accessors.reject do |a|
               config_sections.any? { |section| /\.#{a}\./.match?(section[0]) }
             end
 
@@ -24,23 +44,21 @@ module RailInspector
             config_header = "#### `config.#{accessor}`"
 
             unless config_sections.any? { |section| section[0] == config_header }
-              checker.errors << config_header
+              checker.errors << "Missing configuration: #{config_header}"
               config_sections << [config_header, "", "FIXME", ""]
             end
           end
 
-          checker.doc.general_config =
+          new_config = header + config_sections.sort_by { |section| section[0].split("`")[1] }.flatten
-            [header] +
+
-              config_sections.sort_by { |section| section[0].split("`")[1] }
+          return if new_config == checker.doc.general_config
+
+          checker.errors << "General Configuration is not alphabetical"
+
+          checker.doc.general_config = new_config
         end
 
         private
-          APP_CONFIG_CONST = "Rails::Application::Configuration"
-
-          def app_config_tree
-            checker.parse(APPLICATION_CONFIGURATION_PATH)
-          end
-
           def documented_general_config
             checker
               .doc
@@ -48,19 +66,6 @@ module RailInspector
               .slice_before { |line| line.start_with?("####") }
               .to_a
           end
-
-          def general_accessors
-            visitor.attribute_map[APP_CONFIG_CONST]["attr_accessor"]
-          end
-
-          def visitor
-            @visitor ||=
-              begin
-                visitor = Visitor::Attribute.new
-                visitor.visit(app_config_tree)
-                visitor
-              end
-          end
       end
     end
   end

tools/rail_inspector/test/rail_inspector/configuring/check/general_configuration_test.rb

@@ -0,0 +1,50 @@
+# frozen_string_literal: true
+
+require "test_helper"
+require "rail_inspector/configuring"
+
+class TestGeneralConfiguration < ActiveSupport::TestCase
+  def test_errors_when_configuration_out_of_order
+    with_general_config <<~MD
+      #### `config.b`
+
+      #### `config.a`
+    MD
+
+    check([:a, :b]).check
+
+    assert_not_empty checker.errors
+  end
+
+  def test_no_errors_when_configuration_alphabetical
+    with_general_config <<~MD
+      #### `config.a`
+
+      #### `config.b`
+    MD
+
+    check([:a, :b]).check
+
+    assert_empty checker.errors
+  end
+
+  private
+    def check(expected_accessors)
+      @check ||= RailInspector::Configuring::Check::GeneralConfiguration.new(checker, expected_accessors: expected_accessors)
+    end
+
+    def checker
+      @checker ||= RailInspector::Configuring.new("../..")
+    end
+
+    HEADER = [
+      "### Rails General Configuration",
+      "",
+      "The following configuration methods are to be called on a `Rails::Railtie` object, such as a subclass of `Rails::Engine` or `Rails::Application`.",
+      "",
+    ].freeze
+
+    def with_general_config(markdown)
+      checker.doc.general_config = HEADER + markdown.split("\n")
+    end
+end

tools/rail_inspector/test/test_helper.rb

@@ -0,0 +1,3 @@
+# frozen_string_literal: true
+
+require "active_support"