ronantakizawa/codereview-bench huggingface dataset:
Repository: rails/rails
File: actiontext/lib/action_text/plain_text_conversion.rb (Ruby)

Code before:
      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
      end

      class BottomUpReducer
        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

Review comment (suggestion):
```suggestion
      class BottomUpReducer # :nodoc:
```

https://intrip-actiontext-plaintext-o7gs.rails-docs-preview.pages.dev/api/classes/ActionText/PlainTextConversion/BottomUpReducer

Code after:
      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
      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
Repository: rails/rails
File: actionpack/lib/action_dispatch/http/cache.rb (Ruby)

Code before:
          private
            def parse_directives(header_value)
              return {} unless header_value

              header_value.delete(" ").downcase.split(",").each_with_object({}) do |directive, directives|
                directive_name, directive_value = directive.split("=", 2)

                case
                when SPECIAL_DIRECTIVES.include?(directive_name)
                  directives[directive_name] = directive_value ? directive_value.to_i : true
                when directive_value && VALUE_DIRECTIVES.include?(directive_name)
                  directives[directive_name] = directive_value.to_i
                when BOOLEAN_DIRECTIVES.include?(directive_name)
                  directives[directive_name] = true
                end
              end
            end
        end
      end

      module Response
        attr_reader :cache_control

        def last_modified
          if last = get_header(LAST_MODIFIED)
            Time.httpdate(last)
          end
        end

        def last_modified?
          has_header? LAST_MODIFIED
        end

        def last_modified=(utc_time)
          set_header LAST_MODIFIED, utc_time.httpdate
        end

        def date
          if date_header = get_header(DATE)
            Time.httpdate(date_header)
          end
        end

        def date?
          has_header? DATE
        end

        def date=(utc_time)
          set_header DATE, utc_time.httpdate
        end


Review comment (suggestion):
Rather than to decorate a Hash, I think it would be better to store individual values in instance variables.

Code after:
            def parse_directives(header_value)
              return unless header_value

              header_value.delete(" ").downcase.split(",").each do |directive|
                name, value = directive.split("=", 2)

                case name
                when "max-age"
                  @max_age = value.to_i
                when "min-fresh"
                  @min_fresh = value.to_i
                when "stale-if-error"
                  @stale_if_error = value.to_i
                when "no-cache"
                  @no_cache = true
                when "no-store"
                  @no_store = true
                when "no-transform"
                  @no_transform = true
                when "only-if-cached"
                  @only_if_cached = true
                when "max-stale"
                  @max_stale = value ? value.to_i : true
                end
              end
            end
        end
      end

      module Response
        attr_reader :cache_control

        def last_modified
          if last = get_header(LAST_MODIFIED)
            Time.httpdate(last)
          end
        end

        def last_modified?
          has_header? LAST_MODIFIED
        end

        def last_modified=(utc_time)
          set_header LAST_MODIFIED, utc_time.httpdate
        end

        def date
          if date_header = get_header(DATE)
            Time.httpdate(date_header)
          end
        end
Repository: rails/rails
File: activerecord/test/models/cpk/comment.rb (Ruby)

Code before:
# frozen_string_literal: true

module Cpk
  class Comment < ActiveRecord::Base
    self.table_name = :cpk_comments
    belongs_to :commentable, class_name: "Cpk::Post", foreign_key: %i[commentable_title commentable_author], polymorphic: true
    belongs_to :post, class_name: "Cpk::Post", foreign_key: %i[commentable_title commentable_author]
  end
end


Review comment (suggestion):
Hey @fxn, I was curious why `class_name` was removed from `belong_to :post` since it doesn't have `polymorphic: true` (I tried locally and it didn't raise or warn when present).

Code after:
# frozen_string_literal: true

module Cpk
  class Comment < ActiveRecord::Base
    self.table_name = :cpk_comments
    belongs_to :commentable, foreign_key: %i[commentable_title commentable_author], polymorphic: true
    belongs_to :post, foreign_key: %i[commentable_title commentable_author]
  end
end
Repository: rails/rails
File: actioncable/test/stubs/test_adapter.rb (Ruby)

Code before:
# frozen_string_literal: true

class SuccessAdapter < ActionCable::SubscriptionAdapter::Base
  def broadcast(channel, payload)
  end

  def subscribe(channel, callback, success_callback = nil)
    subscriber_map[channel] << callback
    @@subscribe_called = { channel: channel, callback: callback, success_callback: success_callback }
  end

  def unsubscribe(channel, callback)
    sleep ENV['UNSUBSCRIBE_SLEEP_TIME'].to_f if ENV['UNSUBSCRIBE_SLEEP_TIME']
    subscriber_map[channel].delete(callback)
    subscriber_map.delete(channel) if subscriber_map[channel].empty?
    @@unsubscribe_called = { channel: channel, callback: callback }
  end

  def subscriber_map
    @subscribers ||= Hash.new { |h, k| h[k] = [] }
  end
end


Review comment (refactor):
[nitpick] Consider extracting the sleep duration into a descriptive constant or helper method to clarify its purpose in simulating a delay.

Code after:
# frozen_string_literal: true

class SuccessAdapter < ActionCable::SubscriptionAdapter::Base
  attr_accessor :unsubscribe_latency

  def initialize(...)
    super
    @unsubscribe_latency = nil
  end

  def broadcast(channel, payload)
  end

  def subscribe(channel, callback, success_callback = nil)
    subscriber_map[channel] << callback
    @@subscribe_called = { channel: channel, callback: callback, success_callback: success_callback }
  end

  def unsubscribe(channel, callback)
    sleep @unsubscribe_latency if @unsubscribe_latency
    subscriber_map[channel].delete(callback)
    subscriber_map.delete(channel) if subscriber_map[channel].empty?
    @@unsubscribe_called = { channel: channel, callback: callback }
  end

  def subscriber_map
    @subscribers ||= Hash.new { |h, k| h[k] = [] }
  end
end
Repository: rails/rails
File: activerecord/lib/active_record/filter_attribute_handler.rb (Ruby)

Code before:
# frozen_string_literal: true

module ActiveRecord
  class FilterAttributeHandler # :nodoc:
    class << self
      def on_sensitive_attribute_declared(&block)
        @encrypted_attribute_declaration_listeners ||= Concurrent::Array.new
        @encrypted_attribute_declaration_listeners << block
      end

      def sensitive_attribute_was_declared(klass, list)
        @encrypted_attribute_declaration_listeners&.each do |block|
          block.call(klass, list)
        end
      end
    end

    def initialize(app)
      @app = app
      @attributes_by_class = Concurrent::Map.new
      @collecting = true

      install_collecting_hook
    end

    def enable
      apply_collected_attributes
      @collecting = false
    end

    private
      attr_reader :app

Review comment (security):
This isn't dependent on the encrypted attributes right? So I think these should be renamed to something like `sensitive_attribute_declaration_listeners`. 

Code after:
# frozen_string_literal: true

module ActiveRecord
  class FilterAttributeHandler # :nodoc:
    class << self
      def on_sensitive_attribute_declared(&block)
        @sensitive_attribute_declaration_listeners ||= Concurrent::Array.new
        @sensitive_attribute_declaration_listeners << block
      end

      def sensitive_attribute_was_declared(klass, list)
        @sensitive_attribute_declaration_listeners&.each do |block|
          block.call(klass, list)
        end
      end
    end

    def initialize(app)
      @app = app
      @attributes_by_class = Concurrent::Map.new
      @collecting = true
    end

    def enable
      install_collecting_hook

      apply_collected_attributes
      @collecting = false
    end

    private
      attr_reader :app
Repository: rails/rails
File: activerecord/lib/active_record/filter_attribute_handler.rb (Ruby)

Code before:
# frozen_string_literal: true

module ActiveRecord
  class FilterAttributeHandler # :nodoc:
    class << self
      def on_sensitive_attribute_declared(&block)
        @encrypted_attribute_declaration_listeners ||= Concurrent::Array.new
        @encrypted_attribute_declaration_listeners << block
      end

      def sensitive_attribute_was_declared(klass, list)
        @encrypted_attribute_declaration_listeners&.each do |block|
          block.call(klass, list)
        end
      end
    end

    def initialize(app)
      @app = app
      @attributes_by_class = Concurrent::Map.new
      @collecting = true

      install_collecting_hook
    end

    def enable
      apply_collected_attributes
      @collecting = false
    end

    private
      attr_reader :app

      def install_collecting_hook
        self.class.on_sensitive_attribute_declared do |klass, list|
          attribute_was_declared(klass, list)
        end
      end

      def attribute_was_declared(klass, list)
        if collecting?
          collect_for_later(klass, list)
        else
          apply_filter(klass, list)
        end
      end

      def apply_collected_attributes

Review comment (suggestion):
I think `install_collecting_hook` should be in `#enable`, but that's a person preference and non-blocking. it's a littl cleaner to have the initializer do inits and the `enable` call do the work.

Code after:
# frozen_string_literal: true

module ActiveRecord
  class FilterAttributeHandler # :nodoc:
    class << self
      def on_sensitive_attribute_declared(&block)
        @sensitive_attribute_declaration_listeners ||= Concurrent::Array.new
        @sensitive_attribute_declaration_listeners << block
      end

      def sensitive_attribute_was_declared(klass, list)
        @sensitive_attribute_declaration_listeners&.each do |block|
          block.call(klass, list)
        end
      end
    end

    def initialize(app)
      @app = app
      @attributes_by_class = Concurrent::Map.new
      @collecting = true
    end

    def enable
      install_collecting_hook

      apply_collected_attributes
      @collecting = false
    end

    private
      attr_reader :app

      def install_collecting_hook
        self.class.on_sensitive_attribute_declared do |klass, list|
          attribute_was_declared(klass, list)
        end
      end

      def attribute_was_declared(klass, list)
        if collecting?
          collect_for_later(klass, list)
        else
          apply_filter(klass, list)
        end
      end

      def apply_collected_attributes
Repository: rails/rails
File: actionpack/lib/action_dispatch.rb (Ruby)

Code before:
      autoload :QueryParser
      autoload :Request
      autoload :Response
    end
  end

  autoload_under "middleware" do
    autoload :AssumeSSL
    autoload :HostAuthorization
    autoload :RequestId
    autoload :Callbacks
    autoload :Cookies
    autoload :ActionableExceptions
    autoload :DebugExceptions
    autoload :DebugLocks
    autoload :DebugView
    autoload :ExceptionWrapper
    autoload :Executor
    autoload :Flash
    autoload :PublicExceptions
    autoload :Reloader
    autoload :RemoteIp
    autoload :ServerTiming
    autoload :ShowExceptions
    autoload :TraceToFileExtractor
    autoload :SSL
    autoload :Static
  end

  autoload :Constants
  autoload :Journey
  autoload :MiddlewareStack, "action_dispatch/middleware/stack"
  autoload :Routing

  module Http
    extend ActiveSupport::Autoload

    autoload :Cache
    autoload :Headers
    autoload :MimeNegotiation
    autoload :Parameters
    autoload :UploadedFile, "action_dispatch/http/upload"
    autoload :URL
  end

  module Session
    autoload :AbstractStore,       "action_dispatch/middleware/session/abstract_store"
    autoload :AbstractSecureStore, "action_dispatch/middleware/session/abstract_store"
    autoload :CookieStore,         "action_dispatch/middleware/session/cookie_store"
    autoload :MemCacheStore,       "action_dispatch/middleware/session/mem_cache_store"
    autoload :CacheStore,          "action_dispatch/middleware/session/cache_store"

Review comment (refactor):
```suggestion
    autoload :SSL
    autoload :Static
    autoload :TraceToFileExtractor
```

Knit: to put the autoload at the end (in alphabetical order) also for a cleaner diff

Code after:
      autoload :QueryParser
      autoload :Request
      autoload :Response
    end
  end

  autoload_under "middleware" do
    autoload :AssumeSSL
    autoload :HostAuthorization
    autoload :RequestId
    autoload :Callbacks
    autoload :Cookies
    autoload :ActionableExceptions
    autoload :DebugExceptions
    autoload :DebugLocks
    autoload :DebugView
    autoload :ExceptionWrapper
    autoload :Executor
    autoload :Flash
    autoload :PublicExceptions
    autoload :Reloader
    autoload :RemoteIp
    autoload :ServerTiming
    autoload :ShowExceptions
    autoload :SSL
    autoload :Static
  end

  autoload :Constants
  autoload :Journey
  autoload :MiddlewareStack, "action_dispatch/middleware/stack"
  autoload :Routing

  module Http
    extend ActiveSupport::Autoload

    autoload :Cache
    autoload :Headers
    autoload :MimeNegotiation
    autoload :Parameters
    autoload :UploadedFile, "action_dispatch/http/upload"
    autoload :URL
  end

  module Session
    autoload :AbstractStore,       "action_dispatch/middleware/session/abstract_store"
    autoload :AbstractSecureStore, "action_dispatch/middleware/session/abstract_store"
    autoload :CookieStore,         "action_dispatch/middleware/session/cookie_store"
    autoload :MemCacheStore,       "action_dispatch/middleware/session/mem_cache_store"
    autoload :CacheStore,          "action_dispatch/middleware/session/cache_store"
Repository: rails/rails
File: actionpack/lib/action_dispatch.rb (Ruby)

Code before:
      autoload :ParamError
      autoload :ParameterTypeError, "action_dispatch/http/param_error"
      autoload :ParamsTooDeepError, "action_dispatch/http/param_error"
      autoload :PermissionsPolicy
      autoload :QueryParser
      autoload :Request
      autoload :Response
    end
  end

  autoload_under "middleware" do
    autoload :ActionableExceptions
    autoload :AssumeSSL
    autoload :Callbacks
    autoload :Cookies
    autoload :DebugExceptions
    autoload :DebugLocks
    autoload :DebugView
    autoload :ExceptionWrapper
    autoload :Executor
    autoload :Flash
    autoload :HostAuthorization
    autoload :PublicExceptions
    autoload :Reloader
    autoload :RemoteIp
    autoload :RequestId
    autoload :ServerTiming
    autoload :ShowExceptions
    autoload :SSL
    autoload :Static
    autoload :TraceToFileExtractor
  end

  autoload :Constants
  autoload :Journey
  autoload :MiddlewareStack, "action_dispatch/middleware/stack"
  autoload :Routing

  module Http
    extend ActiveSupport::Autoload

    autoload :Cache
    autoload :Headers
    autoload :MimeNegotiation
    autoload :Parameters
    autoload :UploadedFile, "action_dispatch/http/upload"
    autoload :URL
  end

  module Session
    autoload :AbstractStore,       "action_dispatch/middleware/session/abstract_store"

Review comment (refactor):
Please don't change autoloads unrelated to this PR. I only meant, that since you were adding `TraceToFileExtractor` it made sense to put at the end.

Code after:
      autoload :ParamError
      autoload :ParameterTypeError, "action_dispatch/http/param_error"
      autoload :ParamsTooDeepError, "action_dispatch/http/param_error"
      autoload :PermissionsPolicy
      autoload :QueryParser
      autoload :Request
      autoload :Response
    end
  end

  autoload_under "middleware" do
    autoload :AssumeSSL
    autoload :HostAuthorization
    autoload :RequestId
    autoload :Callbacks
    autoload :Cookies
    autoload :ActionableExceptions
    autoload :DebugExceptions
    autoload :DebugLocks
    autoload :DebugView
    autoload :ExceptionWrapper
    autoload :Executor
    autoload :Flash
    autoload :PublicExceptions
    autoload :Reloader
    autoload :RemoteIp
    autoload :ServerTiming
    autoload :ShowExceptions
    autoload :SSL
    autoload :Static
  end

  autoload :Constants
  autoload :Journey
  autoload :MiddlewareStack, "action_dispatch/middleware/stack"
  autoload :Routing

  module Http
    extend ActiveSupport::Autoload

    autoload :Cache
    autoload :Headers
    autoload :MimeNegotiation
    autoload :Parameters
    autoload :UploadedFile, "action_dispatch/http/upload"
    autoload :URL
  end

  module Session
    autoload :AbstractStore,       "action_dispatch/middleware/session/abstract_store"
    autoload :AbstractSecureStore, "action_dispatch/middleware/session/abstract_store"
Repository: rails/rails
File: railties/lib/rails/generators/app_base.rb (Ruby)

Code before:

      def add_bundler_platforms
        if bundle_install? && !jruby?
          # The vast majority of Rails apps will be deployed on `x86_64-linux`.
          bundle_command("lock --add-platform=x86_64-linux")

          # Users that develop on M1 mac may use docker and would need `aarch64-linux` as well.
          bundle_command("lock --add-platform=aarch64-linux") if RUBY_PLATFORM.start_with?("arm64")
        end
      end

      def jruby?
        defined?(JRUBY_VERSION)
      end

      def empty_directory_with_keep_file(destination, config = {})
        empty_directory(destination, config)
        keep_file(destination)
      end

      def keep_file(destination)
        create_file("#{destination}/.keep") if keeps?
      end

      def user_default_branch
        @user_default_branch ||=
          begin
            `git config init.defaultbranch`.strip
          rescue SystemCallError
            nil
          end
      end

      def git_init_command
        return "git init" if user_default_branch.present?

        git_version = `git --version`[/\d+\.\d+\.\d+/]

        if Gem::Version.new(git_version) >= Gem::Version.new("2.28.0")
          "git init -b main"
        else
          "git init && git symbolic-ref HEAD refs/heads/main"
        end
      end

      def edge_branch
        self.class.edge_branch
      end

      def dockerfile_chown_directories
        directories = %w(log tmp)

Review comment (suggestion):
```suggestion
        @user_default_branch ||= capture_command('git config init.defaultbranch', /.*/)
```

Code after:
        commands = "solid_cache:install solid_queue:install"
        commands += " solid_cable:install" unless skip_action_cable?

        rails_command commands
      end

      def add_bundler_platforms
        if bundle_install? && !jruby?
          # The vast majority of Rails apps will be deployed on `x86_64-linux`.
          bundle_command("lock --add-platform=x86_64-linux")

          # Users that develop on M1 mac may use docker and would need `aarch64-linux` as well.
          bundle_command("lock --add-platform=aarch64-linux") if RUBY_PLATFORM.start_with?("arm64")
        end
      end

      def jruby?
        defined?(JRUBY_VERSION)
      end

      def empty_directory_with_keep_file(destination, config = {})
        empty_directory(destination, config)
        keep_file(destination)
      end

      def keep_file(destination)
        create_file("#{destination}/.keep") if keeps?
      end

      def user_default_branch
        @user_default_branch ||= capture_command("git config init.defaultbranch").strip.presence || "main"
      end

      def git_init_command
        if capture_command("git config init.defaultbranch").present?
          return "git init"
        end

        git_version = `git --version`[/\d+\.\d+\.\d+/]

        if Gem::Version.new(git_version) >= Gem::Version.new("2.28.0")
          "git init -b main"
        else
          "git init && git symbolic-ref HEAD refs/heads/main"
        end
      end

      def edge_branch
        self.class.edge_branch
      end