ronantakizawa/codereview-bench huggingface dataset:
Repository: rails/rails
File: activesupport/lib/active_support/evented_file_update_checker.rb (Ruby)

Code before:

      @core.updated.true?
    end

    def execute
      @core.updated.make_false
      @block.call
    end

    def execute_if_updated
      if updated?
        yield if block_given?
        execute
        true
      end
    end

    class Core
      attr_reader :updated, :files

      def initialize(files, dirs)
        gem_paths = Gem.path
        @files = files.filter_map { |file|
          next if gem_paths.any? { |gem_path| file.to_s.start_with?(gem_path) }
          Pathname(file).expand_path
        }.to_set

        @dirs = dirs.each_with_object({}) do |(dir, exts), hash|
          next if gem_paths.any? { |gem_path| dir.start_with?(gem_path) }
          hash[Pathname(dir).expand_path] = Array(exts).map { |ext| ext.to_s.sub(/\A\.?/, ".") }.to_set
        end

        @common_path = common_path(@dirs.keys)

        @dtw = directories_to_watch
        @missing = []

        @updated = Concurrent::AtomicBoolean.new(false)
        @mutex = Mutex.new

        start
        # inotify / FSEvents file descriptors are inherited on fork, so
        # we need to reopen them otherwise only the parent or the child
        # will be notified.
        # FIXME: this callback is keeping a reference on the instance
        @after_fork = ActiveSupport::ForkTracker.after_fork { start }
      end

      def finalizer
        proc do
          stop

Review comment (suggestion):
```suggestion
        @files = files.filter_map do |file|
          file = File.expand_path(file)
          file unless file.start_with?(*gem_paths)
        end.to_set
```

Code after:

      @core.updated.true?
    end

    def execute
      @core.updated.make_false
      @block.call
    end

    def execute_if_updated
      if updated?
        yield if block_given?
        execute
        true
      end
    end

    class Core
      attr_reader :updated, :files

      def initialize(files, dirs)
        gem_paths = Gem.path
        files = files.map { |f| Pathname(f).expand_path }
        files.reject! { |f| f.to_s.start_with?(*gem_paths) }
        @files = files.to_set

        @dirs = dirs.each_with_object({}) do |(dir, exts), hash|
          next if dir.start_with?(*gem_paths)
          hash[Pathname(dir).expand_path] = Array(exts).map { |ext| ext.to_s.sub(/\A\.?/, ".") }.to_set
        end

        @common_path = common_path(@dirs.keys)

        @dtw = directories_to_watch
        @missing = []

        @updated = Concurrent::AtomicBoolean.new(false)
        @mutex = Mutex.new

        start
        # inotify / FSEvents file descriptors are inherited on fork, so
        # we need to reopen them otherwise only the parent or the child
        # will be notified.
        # FIXME: this callback is keeping a reference on the instance
        @after_fork = ActiveSupport::ForkTracker.after_fork { start }
      end

      def finalizer
        proc do
          stop
          ActiveSupport::ForkTracker.unregister(@after_fork)
Repository: rails/rails
File: actionpack/lib/action_dispatch/http/content_security_policy.rb (Ruby)

Code before:
      end

      def resolve_source(source, context)
        case source
        when String
          source
        when Symbol
          source.to_s
        when Proc
          if context.nil?
            raise RuntimeError, "Missing context for the dynamic content security policy source: #{source.inspect}"
          else
            resolved = context.instance_exec(&source)
            apply_mappings(Array.wrap(resolved))
          end
        else
          raise RuntimeError, "Unexpected content security policy source: #{source.inspect}"
        end
      end

      def nonce_directive?(directive, nonce_directives)
        nonce_directives.include?(directive)
      end

      def hash_source?(source)
        source.start_with?(*HASH_SOURCE_ALGORITHMS.values)
      end
  end
end


Review comment (suggestion):
... then this just becomes

```
source.start_with?(*HASH_SOURCE_ALGORITHMS)
```

Code after:
        when String
          source
        when Symbol
          source.to_s
        when Proc
          if context.nil?
            raise RuntimeError, "Missing context for the dynamic content security policy source: #{source.inspect}"
          else
            resolved = context.instance_exec(&source)
            apply_mappings(Array.wrap(resolved))
          end
        else
          raise RuntimeError, "Unexpected content security policy source: #{source.inspect}"
        end
      end

      def nonce_directive?(directive, nonce_directives)
        nonce_directives.include?(directive)
      end

      def hash_source?(source)
        source.start_with?(*HASH_SOURCE_ALGORITHM_PREFIXES)
      end
  end
end
Repository: rails/rails
File: activerecord/lib/active_record/connection_adapters/abstract_adapter.rb (Ruby)

Code before:

      def supports_insert_returning?
        false
      end

      def supports_insert_on_duplicate_skip?
        false
      end

      def supports_insert_on_duplicate_update?
        false
      end

      def supports_insert_conflict_target?
        false
      end

      def supports_concurrent_connections?
        true
      end

      def supports_nulls_not_distinct?
        false
      end

      def supports_disabling_use_of_index_for_queries?
        false
      end
      alias_method :supports_enabling_use_of_index_for_queries?, :supports_disabling_use_of_index_for_queries?

      def return_value_after_insert?(column) # :nodoc:
        column.auto_populated?
      end

      def async_enabled? # :nodoc:
        supports_concurrent_connections? &&
          !ActiveRecord.async_query_executor.nil? && !pool.async_executor.nil?
      end

      # This is meant to be implemented by the adapters that support extensions
      def disable_extension(name, **)
      end

      # This is meant to be implemented by the adapters that support extensions
      def enable_extension(name, **)
      end

      # This is meant to be implemented by the adapters that support custom enum types
      def create_enum(...) # :nodoc:
      end


Review comment (suggestion):
This method is a bit verbose -- what about `supports_enabling_indexes?` / `supports_disabling_indexes?` instead? the `for_queries` part is kind of implied IMHO.

Code after:

      def supports_insert_returning?
        false
      end

      def supports_insert_on_duplicate_skip?
        false
      end

      def supports_insert_on_duplicate_update?
        false
      end

      def supports_insert_conflict_target?
        false
      end

      def supports_concurrent_connections?
        true
      end

      def supports_nulls_not_distinct?
        false
      end

      def supports_disabling_indexes?
        false
      end

      def return_value_after_insert?(column) # :nodoc:
        column.auto_populated?
      end

      def async_enabled? # :nodoc:
        supports_concurrent_connections? &&
          !ActiveRecord.async_query_executor.nil? && !pool.async_executor.nil?
      end

      # This is meant to be implemented by the adapters that support extensions
      def disable_extension(name, **)
      end

      # This is meant to be implemented by the adapters that support extensions
      def enable_extension(name, **)
      end

      # This is meant to be implemented by the adapters that support custom enum types
      def create_enum(...) # :nodoc:
      end

      # This is meant to be implemented by the adapters that support custom enum types
Repository: rails/rails
File: activerecord/lib/active_record/connection_adapters/deduplicable.rb (Ruby)

Code before:
# frozen_string_literal: true

module ActiveRecord
  module ConnectionAdapters # :nodoc:
    module Deduplicable
      extend ActiveSupport::Concern

      cattr_accessor :classes
      self.classes = []

      included do
        Deduplicable.classes << self
      end

      module ClassMethods
        def registry
          @registry ||= {}
        end

        def new(*, **)
          super.deduplicate
        end
      end

      def deduplicate
        self.class.registry[self] ||= deduplicated
      end
      alias :-@ :deduplicate

      private
        def deduplicated
          freeze
        end
    end

Review comment (suggestion):
Any non-test reason to keep the list of objects in memory? That seems unnecessary overhead just to be able to test something in the framework.

Code after:
# frozen_string_literal: true

module ActiveRecord
  module ConnectionAdapters # :nodoc:
    module Deduplicable
      extend ActiveSupport::Concern

      module ClassMethods
        def registry
          @registry ||= {}
        end

        def new(*, **)
          super.deduplicate
        end
      end

      def deduplicate
        self.class.registry[self] ||= deduplicated
      end
      alias :-@ :deduplicate

      private
        def deduplicated
          freeze
        end
    end
  end
end
Repository: rails/rails
File: activerecord/lib/active_record/connection_adapters/abstract/quoting.rb (Ruby)

Code before:
        self.class.quote_column_name(column_name)
      end

      # Quotes the table name.
      def quote_table_name(table_name)
        self.class.quote_table_name(table_name)
      end

      # Override to return the quoted table name for assignment. Defaults to
      # table quoting.
      #
      # This works for MySQL where table.column can be used to
      # resolve ambiguity.
      #
      # We override this in the sqlite3 and postgresql adapters to use only
      # the column name (as per syntax requirements).
      def quote_table_name_for_assignment(table, attr)
        quote_table_name("#{table}.#{attr}")
      end

      def quote_default_expression(value, column) # :nodoc:
        if value.is_a?(Proc)
          value.call
        else
          cast_type = column.respond_to?(:cast_type) ? column.cast_type : lookup_cast_type(column.sql_type)
          value = cast_type.serialize(value)
          quote(value)
        end
      end

      def quoted_true
        "TRUE"
      end

      def unquoted_true
        true
      end

      def quoted_false
        "FALSE"
      end

      def unquoted_false
        false
      end

      # Quote date/time values for use in SQL input. Includes microseconds
      # if the value is a Time responding to usec.
      def quoted_date(value)
        if value.acts_like?(:time)
          if default_timezone == :utc

Review comment (suggestion):
This was pre-existing, but I find it weird that we have to use `respond_to?` on a type we're in control of.

Code after:
        self.class.quote_column_name(column_name)
      end

      # Quotes the table name.
      def quote_table_name(table_name)
        self.class.quote_table_name(table_name)
      end

      # Override to return the quoted table name for assignment. Defaults to
      # table quoting.
      #
      # This works for MySQL where table.column can be used to
      # resolve ambiguity.
      #
      # We override this in the sqlite3 and postgresql adapters to use only
      # the column name (as per syntax requirements).
      def quote_table_name_for_assignment(table, attr)
        quote_table_name("#{table}.#{attr}")
      end

      def quote_default_expression(value, column) # :nodoc:
        if value.is_a?(Proc)
          value.call
        else
          # TODO: Remove fetch_cast_type and the need for connection after we release 8.1.
          cast_type = column.respond_to?(:fetch_cast_type) ? column.fetch_cast_type(self) : lookup_cast_type(column.sql_type)
          value = cast_type.serialize(value)
          quote(value)
        end
      end

      def quoted_true
        "TRUE"
      end

      def unquoted_true
        true
      end

      def quoted_false
        "FALSE"
      end

      def unquoted_false
        false
      end

      # Quote date/time values for use in SQL input. Includes microseconds
      # if the value is a Time responding to usec.
      def quoted_date(value)
        if value.acts_like?(:time)
Repository: rails/rails
File: activerecord/lib/active_record/type/internal/timezone.rb (Ruby)

Code before:
# frozen_string_literal: true

module ActiveRecord
  module Type
    module Internal
      module Timezone
        def initialize(timezone: nil, **kwargs)
          super(**kwargs)
          @timezone = timezone
        end

        def is_utc?
          default_timezone == :utc
        end

        def default_timezone
          @timezone || ActiveRecord.default_timezone
        end

        def ==(other)
          super(other) && default_timezone == other.default_timezone
        end
      end
    end
  end
end


Review comment (suggestion):
I think we should consider `default_timezone` may change later, hence strictly compare `@timezone` not `default_timezone`.

We can make a `protected attr_reader :timezone` for that.

Code after:
# frozen_string_literal: true

module ActiveRecord
  module Type
    module Internal
      module Timezone
        def initialize(timezone: nil, **kwargs)
          super(**kwargs)
          @timezone = timezone
        end

        def is_utc?
          default_timezone == :utc
        end

        def default_timezone
          @timezone || ActiveRecord.default_timezone
        end

        def ==(other)
          super(other) && timezone == other.timezone
        end

        protected
          attr_reader :timezone
      end
    end
  end
end
Repository: rails/rails
File: activerecord/lib/active_record/statement_cache.rb (Ruby)

Code before:
          val[i] = connection.quote(value)
        end
        val.join
      end
    end

    class PartialQueryCollector
      attr_accessor :preparable, :retryable

      def initialize
        @parts = []
        @binds = []
      end

      def <<(str)
        @parts << str
        self
      end

      def add_bind(obj, &_)
        @binds << obj
        @parts << Substitute.new
        self
      end

      def add_binds(binds, proc_for_binds = nil, &_)
        @binds.concat proc_for_binds ? binds.map(&proc_for_binds) : binds
        binds.size.times do |i|
          @parts << ", " unless i == 0
          @parts << Substitute.new
        end
        self
      end

      def value
        [@parts, @binds]
      end
    end

    def self.query(sql)
      Query.new(sql)
    end

    def self.partial_query(values)
      PartialQuery.new(values)
    end

    def self.partial_query_collector
      PartialQueryCollector.new
    end


Review comment (suggestion):
We require 3.2 now, so you can use an anonymous block

```suggestion
      def add_binds(binds, proc_for_binds = nil, &)
```

Code after:
          val[i] = connection.quote(value)
        end
        val.join
      end
    end

    class PartialQueryCollector
      attr_accessor :preparable, :retryable

      def initialize
        @parts = []
        @binds = []
      end

      def <<(str)
        @parts << str
        self
      end

      def add_bind(obj, &)
        @binds << obj
        @parts << Substitute.new
        self
      end

      def add_binds(binds, proc_for_binds = nil, &)
        @binds.concat proc_for_binds ? binds.map(&proc_for_binds) : binds
        binds.size.times do |i|
          @parts << ", " unless i == 0
          @parts << Substitute.new
        end
        self
      end

      def value
        [@parts, @binds]
      end
    end

    def self.query(sql)
      Query.new(sql)
    end

    def self.partial_query(values)
      PartialQuery.new(values)
    end

    def self.partial_query_collector
      PartialQueryCollector.new
    end
Repository: rails/rails
File: activerecord/lib/active_record/connection_adapters/pool_config.rb (Ruby)

Code before:
        def disconnect_all!
          INSTANCES.each_key { |c| c.disconnect!(automatic_reconnect: true) }
        end
      end

      def initialize(connection_class, db_config, role, shard)
        super()
        @server_version = nil
        self.connection_class = connection_class
        @db_config = db_config
        @role = role
        @shard = shard
        @pool = nil
        INSTANCES[self] = self
      end

      def server_version(connection)
        @server_version || synchronize { @server_version ||= connection.get_database_version }
      end

      def connection_class=(connection_class)
        case connection_class
        when ConnectionHandler::StringConnectionName
          @connection_class = connection_class
        else
          @connection_class = ConnectionHandler::StringConnectionName.new(connection_class.name, connection_class.primary_class?)
        end
      end

      def connection_name
        @connection_class.name
      end

      def disconnect!(automatic_reconnect: false)
        return unless @pool

        synchronize do
          return unless @pool

          @pool.automatic_reconnect = automatic_reconnect
          @pool.disconnect!
        end

        nil
      end

      def pool
        @pool || synchronize { @pool ||= ConnectionAdapters::ConnectionPool.new(self) }
      end

      def discard_pool!

Review comment (refactor):
There's just one minor thing that bother me a bit, it is that now `@connection_class` is always a `StringConnectionName` instance, hence the name makes little sense.


Feels like we should refactor further. AFAIK this is all private API.

Code after:
        def disconnect_all!
          INSTANCES.each_key { |c| c.disconnect!(automatic_reconnect: true) }
        end
      end

      def initialize(connection_class, db_config, role, shard)
        super()
        @server_version = nil
        self.connection_descriptor = connection_class
        @db_config = db_config
        @role = role
        @shard = shard
        @pool = nil
        INSTANCES[self] = self
      end

      def server_version(connection)
        @server_version || synchronize { @server_version ||= connection.get_database_version }
      end

      def connection_descriptor=(connection_descriptor)
        case connection_descriptor
        when ConnectionHandler::ConnectionDescriptor
          @connection_descriptor = connection_descriptor
        else
          @connection_descriptor = ConnectionHandler::ConnectionDescriptor.new(connection_descriptor.name, connection_descriptor.primary_class?)
        end
      end

      def disconnect!(automatic_reconnect: false)
        return unless @pool

        synchronize do
          return unless @pool

          @pool.automatic_reconnect = automatic_reconnect
          @pool.disconnect!
        end

        nil
      end

      def pool
        @pool || synchronize { @pool ||= ConnectionAdapters::ConnectionPool.new(self) }
      end

      def discard_pool!
        return unless @pool

        synchronize do
          return unless @pool
Repository: rails/rails
File: activesupport/lib/active_support/messages/rotator.rb (Ruby)

Code before:
# frozen_string_literal: true

module ActiveSupport
  module Messages
    module Rotator # :nodoc:
      def initialize(*args, on_rotation: nil, **options)
        super(*args, **options)
        @args = args
        @options = options
        @rotations = []
        @on_rotation = on_rotation
      end

      def rotate(*args, **options)
        fall_back_to build_rotation(*args, **options)
      end

      def fall_back_to(fallback)
        @rotations << fallback
        self
      end

      def read_message(message, on_rotation: @on_rotation, **options)
        if @rotations.empty?
          super(message, **options)
        else
          thrown, error = catch_rotation_error do
            return super(message, **options)
          end

          @rotations.each do |rotation|
            catch_rotation_error do
              value = rotation.read_message(message, **options)
              on_rotation&.call
              return value
            end
          end

          throw thrown, error
        end
      end

      private
        def build_rotation(*args, **options)
          self.class.new(*args, *@args.drop(args.length), **@options, **options)
        end


Review comment (suggestion):
Elaborating on what I said in https://github.com/rails/rails/issues/54357#issuecomment-2616371900, the intended API for <code>MessageVerifier<strong>s</strong></code> was the `#on_rotation` method, not `rotate(on_rotation: ...)`.  Also, the intended API for `MessageVerifier` was `new(on_rotation: ...)`, not `rotate(on_rotation: ...)`.

The idea was that only one `on_rotation` callback is called (not one per verifier), so that callback was specified in a singular way.

Code after:
# frozen_string_literal: true

module ActiveSupport
  module Messages
    module Rotator # :nodoc:
      def initialize(*args, on_rotation: nil, **options)
        super(*args, **options)
        @args = args
        @options = options
        @rotations = []
        @on_rotation = on_rotation
      end

      def rotate(*args, on_rotation: nil, **options)
        @on_rotation = on_rotation if on_rotation
        fall_back_to build_rotation(*args, **options)
      end

      def on_rotation(&on_rotation)
        @on_rotation = on_rotation
        self
      end

      def fall_back_to(fallback)
        @rotations << fallback
        self
      end

      def read_message(message, on_rotation: @on_rotation, **options)
        if @rotations.empty?
          super(message, **options)
        else
          thrown, error = catch_rotation_error do
            return super(message, **options)
          end

          @rotations.each do |rotation|
            catch_rotation_error do
              value = rotation.read_message(message, **options)
              on_rotation&.call
              return value
            end
          end

          throw thrown, error
        end
      end
Repository: rails/rails
File: activesupport/lib/active_support/testing/parallelization.rb (Ruby)

Code before:

      def self.after_fork_hook(&blk)
        @@after_fork_hooks << blk
      end

      cattr_reader :after_fork_hooks

      @@run_cleanup_hooks = []

      def self.run_cleanup_hook(&blk)
        @@run_cleanup_hooks << blk
      end

      cattr_reader :run_cleanup_hooks

      def initialize(worker_count)
        @worker_count = worker_count
        @queue_server = Server.new
        @worker_pool = []
        @url = DRb.start_service("drbunix:", @queue_server).uri
      end

      def before_fork
        Parallelization.before_fork_hooks.each do |cb|
          cb.call
        end
      end

      def start
        before_fork
        @worker_pool = @worker_count.times.map do |worker|
          Worker.new(worker, @url).start
        end
      end

      def <<(work)
        @queue_server << work
      end

      def size
        @worker_count
      end

      def shutdown
        @queue_server.shutdown
        @worker_pool.each { |pid| Process.waitpid pid }
      end
    end
  end
end


Review comment (suggestion):
```suggestion
        Parallelization.before_fork_hooks.each(&cb)
```

/nitpick

Code after:

      def self.after_fork_hook(&blk)
        @@after_fork_hooks << blk
      end

      cattr_reader :after_fork_hooks

      @@run_cleanup_hooks = []

      def self.run_cleanup_hook(&blk)
        @@run_cleanup_hooks << blk
      end

      cattr_reader :run_cleanup_hooks

      def initialize(worker_count)
        @worker_count = worker_count
        @queue_server = Server.new
        @worker_pool = []
        @url = DRb.start_service("drbunix:", @queue_server).uri
      end

      def before_fork
        Parallelization.before_fork_hooks.each(&:cb)
      end

      def start
        before_fork
        @worker_pool = @worker_count.times.map do |worker|
          Worker.new(worker, @url).start
        end
      end

      def <<(work)
        @queue_server << work
      end

      def size
        @worker_count
      end

      def shutdown
        @queue_server.shutdown
        @worker_pool.each { |pid| Process.waitpid pid }
      end
    end
  end
end