ronantakizawa/codereview-bench huggingface dataset:
Repository: rails/rails
File: railties/lib/rails/generators/rails/encryption_key_file/encryption_key_file_generator.rb (Ruby)

Code before:
        if File.exist?(".gitignore")
          unless File.read(".gitignore").include?(ignore)
            log "Ignoring #{ignore} so it won't end up in Git history:"
            log ""
            append_to_file ".gitignore", ignore
            log ""
          end
        else
          log "IMPORTANT: Don't commit #{key_path}. Add this to your ignore file:"
          log ignore, :on_green
          log ""
        end
      end

      def ensure_key_files_are_ignored_silently(key_path, ignore: key_ignore(key_path))
        if File.exist?(".gitignore")
          unless File.read(".gitignore").include?(ignore)
            append_to_file ".gitignore", ignore
          end
        end
      end

      private
        def key_ignore(key_path)
          key_path = Pathname.new(key_path) unless key_path.is_a?(Pathname)
          [ "", "# Ignore key files for decrypting credentials and more.", "/#{key_path.dirname.join("*.key")}", "" ].join("\n")
        end
    end
  end
end


Review comment (security):
```suggestion
          <<~IGNORE.squish
            
            # Ignore key files for decrypting credentials and more.
            /#{key_path.dirname.join("*.key")}
            
          IGNORE
```

You might want to inspect the output to make sure indents, etc line up.

Code after:
        if File.exist?(".gitignore")
          unless File.read(".gitignore").include?(ignore)
            log "Ignoring #{ignore} so it won't end up in Git history:"
            log ""
            append_to_file ".gitignore", ignore
            log ""
          end
        else
          log "IMPORTANT: Don't commit #{key_path}. Add this to your ignore file:"
          log ignore, :on_green
          log ""
        end
      end

      def ensure_key_files_are_ignored_silently(key_path, ignore: key_ignore(key_path))
        if File.exist?(".gitignore")
          unless File.read(".gitignore").include?(ignore)
            append_to_file ".gitignore", ignore
          end
        end
      end

      private
        def key_ignore(key_path)
          key_path = Pathname.new(key_path) unless key_path.is_a?(Pathname)
          <<~IGNORE

            # Ignore key files for decrypting credentials and more.
            /#{key_path.dirname.join("*.key")}

          IGNORE
        end
    end
  end
end
Repository: rails/rails
File: activesupport/lib/active_support/testing/error_reporter_assertions.rb (Ruby)

Code before:
        elsif (report = reports.find { |r| error_class === r.error })
          self.assertions += 1
          report
        else
          message = "Expected a #{error_class.name} to be reported, but none of the " \
            "#{reports.size} reported errors matched:  \n" \
            "#{reports.map { |r| r.error.class.name }.join("\n  ")}"
          assert(false, message)
        end
      end

      # Captures reported errors from within the block that match the given
      # error class.
      #
      #   reports = capture_error_reports(IOError) do
      #     Rails.error.report(IOError.new("Oops"))
      #     Rails.error.report(IOError.new("Oh no"))
      #     Rails.error.report(StandardError.new)
      #   end
      #
      #   assert_equal 2, reports.size
      #   assert_equal "Oops", reports.first.error.message
      #   assert_equal "Oh no", reports.last.error.message
      def capture_error_reports(error_class = StandardError)
        reports = ErrorCollector.record do
          yield
        end

        reports.select { |r| error_class === r.error }
      end
    end
  end
end


Review comment (suggestion):
protip: 
```suggestion
        reports.grep(error_class)
```

Code after:
        elsif (report = reports.find { |r| error_class === r.error })
          self.assertions += 1
          report
        else
          message = "Expected a #{error_class.name} to be reported, but none of the " \
            "#{reports.size} reported errors matched:  \n" \
            "#{reports.map { |r| r.error.class.name }.join("\n  ")}"
          assert(false, message)
        end
      end

      # Captures reported errors from within the block that match the given
      # error class.
      #
      #   reports = capture_error_reports(IOError) do
      #     Rails.error.report(IOError.new("Oops"))
      #     Rails.error.report(IOError.new("Oh no"))
      #     Rails.error.report(StandardError.new)
      #   end
      #
      #   assert_equal 2, reports.size
      #   assert_equal "Oops", reports.first.error.message
      #   assert_equal "Oh no", reports.last.error.message
      def capture_error_reports(error_class = StandardError, &block)
        reports = ErrorCollector.record(&block)
        reports.select { |r| error_class === r.error }
      end
    end
  end
end
Repository: rails/rails
File: activesupport/lib/active_support/testing/error_reporter_assertions.rb (Ruby)

Code before:
          self.assertions += 1
          report
        else
          message = "Expected a #{error_class.name} to be reported, but none of the " \
            "#{reports.size} reported errors matched:  \n" \
            "#{reports.map { |r| r.error.class.name }.join("\n  ")}"
          assert(false, message)
        end
      end

      # Captures reported errors from within the block that match the given
      # error class.
      #
      #   reports = capture_error_reports(IOError) do
      #     Rails.error.report(IOError.new("Oops"))
      #     Rails.error.report(IOError.new("Oh no"))
      #     Rails.error.report(StandardError.new)
      #   end
      #
      #   assert_equal 2, reports.size
      #   assert_equal "Oops", reports.first.error.message
      #   assert_equal "Oh no", reports.last.error.message
      def capture_error_reports(error_class = StandardError)
        reports = ErrorCollector.record do
          yield
        end

        reports.select { |r| error_class === r.error }
      end
    end
  end
end


Review comment (suggestion):
```suggestion
      def capture_error_reports(error_class = StandardError, &block)
        reports = ErrorCollector.record(&block)
```

Code after:
          self.assertions += 1
          report
        else
          message = "Expected a #{error_class.name} to be reported, but none of the " \
            "#{reports.size} reported errors matched:  \n" \
            "#{reports.map { |r| r.error.class.name }.join("\n  ")}"
          assert(false, message)
        end
      end

      # Captures reported errors from within the block that match the given
      # error class.
      #
      #   reports = capture_error_reports(IOError) do
      #     Rails.error.report(IOError.new("Oops"))
      #     Rails.error.report(IOError.new("Oh no"))
      #     Rails.error.report(StandardError.new)
      #   end
      #
      #   assert_equal 2, reports.size
      #   assert_equal "Oops", reports.first.error.message
      #   assert_equal "Oh no", reports.last.error.message
      def capture_error_reports(error_class = StandardError, &block)
        reports = ErrorCollector.record(&block)
        reports.select { |r| error_class === r.error }
      end
    end
  end
end
Repository: rails/rails
File: railties/lib/rails/application/configuration.rb (Ruby)

Code before:
            active_record.validate_migration_timestamps = true
          end
        when "8.0"
          load_defaults "7.2"

          if respond_to?(:active_support)
            active_support.to_time_preserves_timezone = :zone
          end

          if respond_to?(:action_dispatch)
            action_dispatch.strict_freshness = true
          end

          Regexp.timeout ||= 1 if Regexp.respond_to?(:timeout=)
        when "8.1"
          load_defaults "8.0"

          # Development and test environments tend to reload code and
          # redefine methods (e.g. mocking), hence YJIT isn't generally
          # faster in these environments.
          self.yjit = !Rails.env.local?

          if respond_to?(:action_controller)
            ActionController.deprecator.silence do
              action_controller.escape_json_responses = false
            end
          end
        else
          raise "Unknown version #{target_version.to_s.inspect}"
        end

        @loaded_config_version = target_version
      end

      def reloading_enabled?
        enable_reloading
      end

      def enable_reloading
        !cache_classes
      end

      def enable_reloading=(value)
        self.cache_classes = !value
      end

      def encoding=(value)
        @encoding = value
        silence_warnings do
          Encoding.default_external = value
          Encoding.default_internal = value

Review comment (security):
I don't think this is correct, because here `action_controller.escape_json_responses =` isn't the method you defined, it's an `OrderedOption`, the value will be forwarded to `ActionController:Base` from an initializer.

Code after:
            active_record.validate_migration_timestamps = true
          end
        when "8.0"
          load_defaults "7.2"

          if respond_to?(:active_support)
            active_support.to_time_preserves_timezone = :zone
          end

          if respond_to?(:action_dispatch)
            action_dispatch.strict_freshness = true
          end

          Regexp.timeout ||= 1 if Regexp.respond_to?(:timeout=)
        when "8.1"
          load_defaults "8.0"

          # Development and test environments tend to reload code and
          # redefine methods (e.g. mocking), hence YJIT isn't generally
          # faster in these environments.
          self.yjit = !Rails.env.local?

          if respond_to?(:action_controller)
            action_controller.escape_json_responses = false
          end
        else
          raise "Unknown version #{target_version.to_s.inspect}"
        end

        @loaded_config_version = target_version
      end

      def reloading_enabled?
        enable_reloading
      end

      def enable_reloading
        !cache_classes
      end

      def enable_reloading=(value)
        self.cache_classes = !value
      end

      def encoding=(value)
        @encoding = value
        silence_warnings do
          Encoding.default_external = value
          Encoding.default_internal = value
        end
      end
Repository: rails/rails
File: actionview/lib/action_view/helpers/tags/select.rb (Ruby)

Code before:

        def render
          option_tags_options = {
            selected: @options.fetch(:selected) { value.nil? ? "" : value },
            disabled: @options[:disabled]
          }

          option_tags = if grouped_choices?
            grouped_options_for_select(@choices, option_tags_options)
          else
            options_for_select(@choices, option_tags_options)
          end

          select_content_tag(option_tags, @options, @html_options)
        end

        private
          # Grouped choices look like this:
          #
          #   [nil, []]
          #   { nil => [] }
          def grouped_choices?
            return false if @choices.blank?

            first_choice = @choices.first
            return false unless first_choice.is_a?(Array) || first_choice.is_a?(Hash)

            first_choice.second.is_a?(Array)
          end
      end
    end
  end
end


Review comment (suggestion):
~Maybe we can check if it's an Enumerable? It seems that this was the intended behaviour according to https://github.com/rails/rails/commit/efe62b71d52c40849ead8855fb4f42fae91d4c54~

Edit: Sorry, it's not correct actually. The PR mentions the type for `choices` but no mention about the individual items.

Code after:

        def render
          option_tags_options = {
            selected: @options.fetch(:selected) { value.nil? ? "" : value },
            disabled: @options[:disabled]
          }

          option_tags = if grouped_choices?
            grouped_options_for_select(@choices, option_tags_options)
          else
            options_for_select(@choices, option_tags_options)
          end

          select_content_tag(option_tags, @options, @html_options)
        end

        private
          # Grouped choices look like this:
          #
          #   [nil, []]
          #   { nil => [] }
          def grouped_choices?
            return false if @choices.blank?

            first_choice = @choices.first
            return false unless first_choice.is_a?(Enumerable)

            first_choice.second.is_a?(Array)
          end
      end
    end
  end
end
Repository: rails/rails
File: activesupport/lib/active_support/error_reporter.rb (Ruby)

Code before:
          )
        else
          raise
        end
      end

      while error
        unless error.frozen?
          error.instance_variable_set(:@__rails_error_reported, true)
        end
        error = error.cause
      end

      nil
    end

    private
      def ensure_backtrace(error)
        return if error.frozen? # re-raising won't add a backtrace
        return unless error.backtrace.nil?

        begin
          # We could use Exception#set_backtrace, but until Ruby 3.4
          # it only support setting `Exception#backtrace` and not
          # `Exception#backtrace_locations`. So raising the exception
          # is a good way to build a real backtrace.
          raise error
        rescue error.class => error
        end

        count = 0
        while error.backtrace_locations.first&.path == __FILE__
          count += 1
          error.backtrace_locations.shift
        end

        error.backtrace.shift(count)
      end

      class ErrorContextMiddlewareStack # :nodoc:
        def initialize
          @stack = []
        end

        # Add a middleware to the error context stack.
        def use(middleware)
          unless middleware.respond_to?(:call)
            raise ArgumentError, "Error context middleware must respond to #call"
          end

          @stack << middleware

Review comment (suggestion):
@byroot I'm curious, is there any reason to `rescue error.class` specifically? Couldn't we just hardcode:

```suggestion
        rescue Exception => error
```

Code after:
          )
        else
          raise
        end
      end

      while error
        unless error.frozen?
          error.instance_variable_set(:@__rails_error_reported, true)
        end
        error = error.cause
      end

      nil
    end

    private
      def ensure_backtrace(error)
        return if error.frozen? # re-raising won't add a backtrace or set the cause
        return unless error.backtrace.nil?

        begin
          # As of Ruby 3.4, we could use Exception#set_backtrace to set the backtrace,
          # but there's nothing like Exception#set_cause. Raising+rescuing is the only way to set the cause.
          raise error
        rescue error.class => error
        end

        count = 0
        while error.backtrace_locations.first&.path == __FILE__
          count += 1
          error.backtrace_locations.shift
        end

        error.backtrace.shift(count)
      end

      class ErrorContextMiddlewareStack # :nodoc:
        def initialize
          @stack = []
        end

        # Add a middleware to the error context stack.
        def use(middleware)
          unless middleware.respond_to?(:call)
            raise ArgumentError, "Error context middleware must respond to #call"
          end

          @stack << middleware
        end
Repository: rails/rails
File: actiontext/lib/action_text/plain_text_conversion.rb (Ruby)

Code before:

      def plain_text_for_blockquote_node(node)
        text = plain_text_for_block(node)
        return "“”" if text.blank?

        text = text.dup
        text.insert(text.rindex(/\S/) + 1, "”")
        text.insert(text.index(/\S/), "“")
        text
      end

      def plain_text_for_li_node(node)
        bullet = bullet_for_li_node(node)
        text = remove_trailing_newlines(plain_text_for_node_children(node))
        indentation = indentation_for_li_node(node)

        "#{indentation}#{bullet} #{text}\n"
      end

      def remove_trailing_newlines(text)
        text.chomp("")
      end

      def bullet_for_li_node(node)
        if list_node_name_for_li_node(node) == "ol"
          index = node.parent.children.index(node)
          "#{index + 1}."
        else
          "•"
        end
      end

      def list_node_name_for_li_node(node)
        node.ancestors.lazy.map(&:name).grep(/^[uo]l$/).first
      end

      def indentation_for_li_node(node)
        depth = list_node_depth_for_node(node)
        if depth > 1
          "  " * (depth - 1)
        end
      end

      def list_node_depth_for_node(node)
        node.ancestors.map(&:name).grep(/^[uo]l$/).count
      end

      def break_if_nested_list(node, text)
        if list_node_depth_for_node(node) > 0
          "\n#{text}"
        else

Review comment (bug):
This will break if an ordered list has text nodes between the items. It's pretty common to have whitespace, like this:

```html
<ol>
<li>foo</li>
<li>bar</li>
</ol>
```

in which case "foo" will be numbered "2." and "bar" will be numbered "4.".

I'm not sure if Action Text ever formats lists this way, but I wanted to mention it.

Code after:
        text = plain_text_for_block(node, child_values)
        return "“”" if text.blank?

        text = text.dup
        text.insert(text.rindex(/\S/) + 1, "”")
        text.insert(text.index(/\S/), "“")
        text
      end

      def plain_text_for_li_node(node, child_values)
        bullet = bullet_for_li_node(node)
        text = remove_trailing_newlines(plain_text_for_child_values(child_values))
        indentation = indentation_for_li_node(node)

        "#{indentation}#{bullet} #{text}\n"
      end

      def remove_trailing_newlines(text)
        text.chomp("")
      end

      def bullet_for_li_node(node)
        if list_node_name_for_li_node(node) == "ol"
          index = node.parent.elements.index(node)
          "#{index + 1}."
        else
          "•"
        end
      end

      def list_node_name_for_li_node(node)
        node.ancestors.lazy.map(&:name).grep(/^[uo]l$/).first
      end

      def indentation_for_li_node(node)
        depth = list_node_depth_for_node(node)
        if depth > 1
          "  " * (depth - 1)
        end
      end

      def list_node_depth_for_node(node)
        node.ancestors.map(&:name).grep(/^[uo]l$/).count
      end

      def break_if_nested_list(node, text)
        if list_node_depth_for_node(node) > 0
          "\n#{text}"
        else
          text
        end
Repository: rails/rails
File: actiontext/lib/action_text/plain_text_conversion.rb (Ruby)

Code before:
# frozen_string_literal: true

# :markup: markdown

module ActionText
  module PlainTextConversion
    extend self

    def node_to_plain_text(node)
      remove_trailing_newlines(node_to_plain_text_content_tree(node).content)
    end

    private
      def node_to_plain_text_content_tree(node)
        BottomUpReplacer.replace_content(node.dup) { |n| plain_text_for_node(n) }
      end

      def plain_text_for_node(node)
        if respond_to?(plain_text_method_for_node(node), true)
          send(plain_text_method_for_node(node), node)
        else
          plain_text_for_node_children(node)
        end
      end

      def plain_text_for_node_children(node)
        node.children.map(&:content).join
      end

      def plain_text_method_for_node(node)
        :"plain_text_for_#{node.name}_node"
      end

      def plain_text_for_unsupported_node(node)
        ""
      end

      %i[ script style].each do |element|
        alias_method :"plain_text_for_#{element}_node", :plain_text_for_unsupported_node
      end

Review comment (performance):
I just want to note that the `.dup` here will be expensive (in terms of memory used) for large documents.


Code after:
# frozen_string_literal: true

# :markup: markdown

module ActionText
  module PlainTextConversion
    extend self

    def node_to_plain_text(node)
      BottomUpReducer.new(node).reduce do |n, child_values|
        plain_text_for_node(n, child_values)
      end.then(&method(:remove_trailing_newlines))
    end

    private
      def plain_text_for_node(node, child_values)
        if respond_to?(plain_text_method_for_node(node), true)
          send(plain_text_method_for_node(node), node, child_values)
        else
          plain_text_for_child_values(child_values)
        end
      end

      def plain_text_method_for_node(node)
        :"plain_text_for_#{node.name}_node"
      end

      def plain_text_for_child_values(child_values)
        child_values.join
      end

      def plain_text_for_unsupported_node(node, _child_values)
        ""
      end

      %i[ script style].each do |element|
        alias_method :"plain_text_for_#{element}_node", :plain_text_for_unsupported_node
      end

      def plain_text_for_block(node, child_values)
Repository: rails/rails
File: actiontext/lib/action_text/plain_text_conversion.rb (Ruby)

Code before:

      def list_node_depth_for_node(node)
        node.ancestors.map(&:name).grep(/^[uo]l$/).count
      end

      def break_if_nested_list(node, text)
        if list_node_depth_for_node(node) > 0
          "\n#{text}"
        else
          text
        end
      end

      class BottomUpReplacer
        def self.replace_content(node, &block)
          new(node).replace_content(&block)
        end

        def initialize(node)
          @node = node
        end

        def replace_content(&block)
          @node.tap do |node|
            traverse_bottom_up(node) do |n|
              n.content = block.call(n)
            end
          end
        end

        private
          def traverse_bottom_up(node, &block)
            call_stack, processing_stack = [ node ], []

            until call_stack.empty?
              node = call_stack.pop
              processing_stack.push(node)
              call_stack.concat node.children
            end

            processing_stack.reverse_each(&block)
          end
      end
  end
end


Review comment (suggestion):
This is going to create a lot of short-lived strings as the children's plain text equivalents are joined together at each level of the hierarchy. Those strings add to the total compute time as well as cause GC churn.

This becomes noticeable if the "deeply nested" test is modified like:

```patch
   test "deeply nested tags are converted" do
     assert_converted_to(
       "Hello world!\nHow are you?",
-      ActionText::Fragment.wrap("<div>Hello world!</div><div></div>").tap do |fragment|
+      ActionText::Fragment.wrap("<div>Hello world!</div><div>Abc<span>Xyx</span></div>").tap do |fragment|
         node = fragment.source.children.last
         10_000.times do
           child = node.clone
```

and then the time it takes to run jumps from 1.8s to 5.5s on my machine.

Code after:
        node.ancestors.map(&:name).grep(/^[uo]l$/).count
      end

      def break_if_nested_list(node, text)
        if list_node_depth_for_node(node) > 0
          "\n#{text}"
        else
          text
        end
      end

      class BottomUpReducer # :nodoc:
        def initialize(node)
          @node = node
          @values = {}
        end

        def reduce(&block)
          traverse_bottom_up(@node) do |n|
            child_values = @values.values_at(*n.children)
            @values[n] = block.call(n, child_values)
          end
          @values[@node]
        end

        private
          def traverse_bottom_up(node, &block)
            call_stack, processing_stack = [ node ], []

            until call_stack.empty?
              node = call_stack.pop
              processing_stack.push(node)
              call_stack.concat node.children
            end

            processing_stack.reverse_each(&block)
          end
      end
  end
end