ronantakizawa/codereview-bench huggingface dataset:
Repository: rails/rails
File: activestorage/lib/active_storage/downloader.rb (Ruby)

Code before:
# frozen_string_literal: true

module ActiveStorage
  class Downloader # :nodoc:
    attr_reader :service

    def initialize(service)
      @service = service
    end

    def open(key, checksum: nil, verify: true, name: "ActiveStorage-", tmpdir: nil)
      if block_given?
        Tempfile.create(name, tmpdir, binmode: true) do |file|
          download(key, file)
          verify_integrity_of(file, checksum: checksum) if verify
          yield file
        end
      else
        file = Tempfile.create(name, tmpdir, binmode: true)
        download(key, file)
        verify_integrity_of(file, checksum: checksum) if verify
        file
      end
    end

    private
      def download(key, file)
        service.download(key) { |chunk| file.write(chunk) }
        file.flush
        file.rewind
      end

      def verify_integrity_of(file, checksum:)
        unless OpenSSL::Digest::MD5.file(file).base64digest == checksum
          raise ActiveStorage::IntegrityError
        end
      end
  end

Review comment (suggestion):
I'm not sure but do `Tempfile.create(&block)` and `Tempfile.open` behave differently?

I think there is a way to write this and not change that, which might be worth investigating.

Code after:
# frozen_string_literal: true

module ActiveStorage
  class Downloader # :nodoc:
    attr_reader :service

    def initialize(service)
      @service = service
    end

    def open(key, checksum: nil, verify: true, name: "ActiveStorage-", tmpdir: nil, &block)
      tempfile = Tempfile.new(name, tmpdir, binmode: true)
      download(key, tempfile)
      verify_integrity_of(tempfile, checksum: checksum) if verify

      if block_given?
        begin
          yield tempfile
        ensure
          tempfile.close!
        end
      else
        tempfile
      end
    end

    private
      def download(key, file)
        service.download(key) { |chunk| file.write(chunk) }
        file.flush
        file.rewind
      end

      def verify_integrity_of(file, checksum:)
        unless OpenSSL::Digest::MD5.file(file).base64digest == checksum
          raise ActiveStorage::IntegrityError
        end
      end
Repository: rails/rails
File: activestorage/app/jobs/active_storage/transform_job.rb (Ruby)

Code before:
# frozen_string_literal: true

class ActiveStorage::TransformJob < ActiveStorage::BaseJob
  queue_as { ActiveStorage.queues[:transform] }

  discard_on ActiveRecord::RecordNotFound
  retry_on ActiveStorage::IntegrityError, attempts: 10, wait: :polynomially_longer

  def perform(blob, transformations)
    blob.send(blob.previewable? ? :preview : :variant, transformations).processed
  end
end


Review comment (suggestion):
[`representation`](https://api.rubyonrails.org/v7.1/classes/ActiveStorage/Blob/Representable.html#method-i-representation) implements this logic internally.

```suggestion
    blob.representation(transformations).processed
```

Code after:
# frozen_string_literal: true

class ActiveStorage::TransformJob < ActiveStorage::BaseJob
  queue_as { ActiveStorage.queues[:transform] }

  discard_on ActiveRecord::RecordNotFound
  retry_on ActiveStorage::IntegrityError, attempts: 10, wait: :polynomially_longer

  def perform(blob, transformations)
    blob.representation(transformations).processed
  end
end
Repository: rails/rails
File: actionpack/lib/action_dispatch/routing/routes_proxy.rb (Ruby)

Code before:

      def url_options
        scope.send(:_with_routes, routes) do
          scope.url_options
        end
      end

    private
      def respond_to_missing?(method, _)
        super || @helpers.respond_to?(method)
      end

      def method_missing(method, *args)
        if @helpers.respond_to?(method)
          options = args.extract_options!
          options = url_options.merge((options || {}).symbolize_keys)

          if @script_namer
            options[:script_name] = merge_script_names(
              options[:script_name],
              @script_namer.call(options)
            )
          end

          args << options
          @helpers.send(method, *args)
        else
          super
        end
      end

      # Keeps the part of the script name provided by the global
      # context via ENV["SCRIPT_NAME"], which `mount` doesn't know
      # about since it depends on the specific request, but use our
      # script name resolver for the mount point dependent part.
      def merge_script_names(previous_script_name, new_script_name)
        return new_script_name unless previous_script_name

        resolved_parts = new_script_name.count("/")
        previous_parts = previous_script_name.count("/")
        context_parts = previous_parts - resolved_parts + 1

        (previous_script_name.split("/").slice(0, context_parts).join("/")) + new_script_name
      end
    end
  end
end


Review comment (suggestion):
Should this be `public_send`?  It looks like the previously eval'd code would generate a "public" call.

Code after:

      def url_options
        scope.send(:_with_routes, routes) do
          scope.url_options
        end
      end

    private
      def respond_to_missing?(method, _)
        super || @helpers.respond_to?(method)
      end

      def method_missing(method, *args)
        if @helpers.respond_to?(method)
          options = args.extract_options!
          options = url_options.merge((options || {}).symbolize_keys)

          if @script_namer
            options[:script_name] = merge_script_names(
              options[:script_name],
              @script_namer.call(options)
            )
          end

          args << options
          @helpers.public_send(method, *args)
        else
          super
        end
      end

      # Keeps the part of the script name provided by the global
      # context via ENV["SCRIPT_NAME"], which `mount` doesn't know
      # about since it depends on the specific request, but use our
      # script name resolver for the mount point dependent part.
      def merge_script_names(previous_script_name, new_script_name)
        return new_script_name unless previous_script_name

        resolved_parts = new_script_name.count("/")
        previous_parts = previous_script_name.count("/")
        context_parts = previous_parts - resolved_parts + 1

        (previous_script_name.split("/").slice(0, context_parts).join("/")) + new_script_name
      end
    end
  end
end
Repository: rails/rails
File: actionpack/lib/action_dispatch/http/url.rb (Ruby)

Code before:
# frozen_string_literal: true

require "active_support/core_ext/module/attribute_accessors"

module ActionDispatch
  module Http
    module URL
      IP_HOST_REGEXP  = /\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}$/
      HOST_REGEXP     = /(^[^:]+:\/\/)?(\[[^\]]+\]|[^:]+)(?::(\d+$))?/
      PROTOCOL_REGEXP = /^([^:]+)(:)?(\/\/)?$/

      mattr_accessor :secure_protocol, default: false
      mattr_accessor :tld_length, default: 1
      mattr_accessor :domain_extractor

      class DomainExtractor # :nodoc:
        attr_reader :host, :tld_length

        def initialize(host, tld_length)
          @host, @tld_length = host, tld_length
        end

        def domain
          host.split(".").last(1 + tld_length).join(".")
        end

        def subdomains
          parts = host.split(".")
          parts[0..-(tld_length + 2)]
        end
      end

      class << self
        # Returns the domain part of a host given the domain level.
        #
        #    # Top-level domain example
        #    extract_domain('www.example.com', 1) # => "example.com"
        #    # Second-level domain example
        #    extract_domain('dev.www.example.co.uk', 2) # => "example.co.uk"

Review comment (refactor):
```suggestion
      mattr_accessor :domain_extractor, default: DomainExtractor
```

Code after:
# frozen_string_literal: true

# :markup: markdown

require "active_support/core_ext/module/attribute_accessors"

module ActionDispatch
  module Http
    module URL
      IP_HOST_REGEXP  = /\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}$/
      HOST_REGEXP     = /(^[^:]+:\/\/)?(\[[^\]]+\]|[^:]+)(?::(\d+$))?/
      PROTOCOL_REGEXP = /^([^:]+)(:)?(\/\/)?$/

      # DomainExtractor provides utility methods for extracting domain and subdomain
      # information from host strings. This module is used internally by Action Dispatch
      # to parse host names and separate the domain from subdomains based on the
      # top-level domain (TLD) length.
      #
      # The module assumes a standard domain structure where domains consist of:
      # - Subdomains (optional, can be multiple levels)
      # - Domain name
      # - Top-level domain (TLD, can be multiple levels like .co.uk)
      #
      # For example, in "api.staging.example.co.uk":
      # - Subdomains: ["api", "staging"]
      # - Domain: "example.co.uk" (with tld_length=2)
      # - TLD: "co.uk"
      module DomainExtractor
        extend self

        # Extracts the domain part from a host string, including the specified
        # number of top-level domain components.
        #
        # The domain includes the main domain name plus the TLD components.
        # The +tld_length+ parameter specifies how many components from the right
        # should be considered part of the TLD.
        #
        # ==== Parameters
        #
Repository: rails/rails
File: activestorage/app/jobs/active_storage/create_variants_job.rb (Ruby)

Code before:
# frozen_string_literal: true

class ActiveStorage::CreateVariantsJob < ActiveStorage::BaseJob
  queue_as { ActiveStorage.queues[:transform] }

  discard_on ActiveRecord::RecordNotFound
  retry_on ActiveStorage::IntegrityError, attempts: 10, wait: :polynomially_longer

  def perform(blob, transformations:, generate:)
    @blob = blob
    @generate = generate

    @blob.preview({}).processed if preview_image_needed?

    transformations.each do |transformation|
      ActiveStorage::TransformJob.public_send(perform_method, @blob, transformation)
    end
  end

  private
    def perform_method
      (@generate == :immediate) ? :perform_now : :perform_later
    end

    def preview_image_needed?
      @blob.previewable? && !@blob.preview_image.attached?
    end
end


Review comment (suggestion):
The job uses the `@generate` to determine whether it needs to perform the `TransformJob`s now or later. I couldn't find a way for the job itself to know whether it is being run with a `perform_now` or a `perform_later`, so I passed it explicitly to the job.

Code after:
# frozen_string_literal: true

class ActiveStorage::CreateVariantsJob < ActiveStorage::BaseJob
  queue_as { ActiveStorage.queues[:transform] }

  discard_on ActiveRecord::RecordNotFound
  retry_on ActiveStorage::IntegrityError, attempts: 10, wait: :polynomially_longer

  def perform(blob, variants:, process:)
    @blob = blob
    @process = process

    create_preview_image if preview_image_needed?

    variants.each do |transformations|
      ActiveStorage::TransformJob.public_send(perform_method, @blob, transformations)
    end
  end

  private
    def create_preview_image
      @blob.preview({}).processed
    end

    def preview_image_needed?
      @blob.previewable? && !@blob.preview_image.attached?
    end

    def perform_method
      (@process == :immediately) ? :perform_now : :perform_later
    end
end
Repository: rails/rails
File: activestorage/app/jobs/active_storage/transform_job.rb (Ruby)

Code before:
# frozen_string_literal: true

class ActiveStorage::TransformJob < ActiveStorage::BaseJob
  queue_as { ActiveStorage.queues[:transform] }

  discard_on ActiveRecord::RecordNotFound
  retry_on ActiveStorage::IntegrityError, attempts: 10, wait: :polynomially_longer

  def perform(blob, transformation)
    blob.representation(transformation).processed
  end
end


Review comment (suggestion):
`TransformJob` is only called from one place and should not be called with an unrepresentable blob. Rather than discarding, I think it should raise an exception if someone were to break this.

Code after:
# frozen_string_literal: true

class ActiveStorage::TransformJob < ActiveStorage::BaseJob
  queue_as { ActiveStorage.queues[:transform] }

  discard_on ActiveRecord::RecordNotFound
  retry_on ActiveStorage::IntegrityError, attempts: 10, wait: :polynomially_longer

  def perform(blob, transformations)
    blob.representation(transformations).processed
  end
end
Repository: rails/rails
File: railties/test/application/console_test.rb (Ruby)

Code before:
  end

  def test_rails_console_methods_patch_with_module_inclusion_deprecation_caller_location
    add_to_config <<-RUBY
      module MyConsole
        def foo
          "this is foo"
        end
      end

      console do
        ::Rails::ConsoleMethods.include(MyConsole)
      end
    RUBY

    spawn_console("-e development", wait_for_prompt: false)

    line_number = 0
    app = File.readlines("#{app_path}/config/application.rb")
    app.each_with_index do |line, index|
      if line.include?("Rails::ConsoleMethods.include(MyConsole)")
        line_number = index + 1
      end
    end

    assert_output "(called from block in <class:Application> at #{app_path}/config/application.rb:#{line_number})", @primary, 30
  end

  def test_reload_command_reload_constants
    app_file "app/models/user.rb", <<-MODEL
      class User
        attr_accessor :name
      end
    MODEL

    options = "-e development"
    # Now the User model has only one attribute called `name`
    spawn_console(options)


    write_prompt "User.new.respond_to?(:age)", "=> false"

    # This will be loaded after the reload! command is executed
    app_file "app/models/user.rb", <<-MODEL
      class User
        attr_accessor :name, :age
      end
    MODEL

    write_prompt "reload!", "Reloading...\r\n"
    write_prompt "User.new.respond_to?(:age)", "=> true"

Review comment (performance):
Let's just merge this additional assertion with `test_rails_console_methods_patch_backward_compatibility_with_module_inclusion` to keep fewer of these slow tests around.

Code after:
          end
        end
      end
    RUBY

    spawn_console("-e development", wait_for_prompt: false)

    assert_output "Extending Rails console through `Rails::ConsoleMethods` is deprecated", @primary, 30
    write_prompt "foo", "=> \"this is foo\""
  end

  def test_reload_command_reload_constants
    app_file "app/models/user.rb", <<-MODEL
      class User
        attr_accessor :name
      end
    MODEL

    options = "-e development"
    # Now the User model has only one attribute called `name`
    spawn_console(options)


    write_prompt "User.new.respond_to?(:age)", "=> false"

    # This will be loaded after the reload! command is executed
    app_file "app/models/user.rb", <<-MODEL
      class User
        attr_accessor :name, :age
      end
    MODEL

    write_prompt "reload!", "Reloading...\r\n"
    write_prompt "User.new.respond_to?(:age)", "=> true"
  end

  def test_console_respects_user_defined_prompt_mode
    irbrc = Tempfile.new("irbrc")
    irbrc.write <<-RUBY
      IRB.conf[:PROMPT_MODE] = :SIMPLE
    RUBY
    irbrc.close

    options = "-e test"
    spawn_console(options, env: { "IRBRC" => irbrc.path })

    write_prompt "123", prompt: ">> "
  ensure
    File.unlink(irbrc)
  end
Repository: rails/rails
File: actionpack/test/dispatch/routing/log_subscriber_test.rb (Ruby)

Code before:
    draw do
      get "redirect", to: redirect("/login")
    end

    get "/redirect"
    wait

    assert_equal 2, logs.size
    assert_equal "Redirected to http://www.example.com/login", logs.first
    assert_match(/Completed 301/, logs.last)
  end

  test "verbose redirect logs" do
    ActionDispatch.verbose_redirect_logs = true

    draw do
      get "redirect", to: redirect("/login")
    end

    get "/redirect"
    wait

    puts logs

    assert_equal 3, logs.size
    assert_match(/↳/, logs[1])
  ensure
    ActionDispatch.verbose_redirect_logs = false
  end

  private
    def draw(&block)
      self.class.stub_controllers do |routes|
        routes.default_url_options = { host: "www.example.com" }
        routes.draw(&block)
        @app = RoutedRackApp.new routes
      end
    end

    def logs
      @logs ||= @logger.logged(:info)
    end
end


Review comment (suggestion):
One way to get this to pass is to override the `get` method in this test class:

```patch
diff --git a/actionpack/test/dispatch/routing/log_subscriber_test.rb b/actionpack/test/dispatch/routing/log_subscriber_test.rb
index c28676ec..daf828ae 100644
--- a/actionpack/test/dispatch/routing/log_subscriber_test.rb
+++ b/actionpack/test/dispatch/routing/log_subscriber_test.rb
@@ -52,6 +52,10 @@ def draw(&block)
       end
     end
 
+    def get(path, **options)
+      super(path, **options.merge(headers: { "action_dispatch.routes" => @app.routes }))
+    end
+
     def logs
       @logs ||= @logger.logged(:info)
     end
```

Code after:
    draw do
      get "redirect", to: redirect("/login")
    end

    get "/redirect"
    wait

    assert_equal 2, logs.size
    assert_equal "Redirected to http://www.example.com/login", logs.first
    assert_match(/Completed 301/, logs.last)
  end

  test "verbose redirect logs" do
    old_cleaner = ActionDispatch::LogSubscriber.backtrace_cleaner
    ActionDispatch::LogSubscriber.backtrace_cleaner = ActionDispatch::LogSubscriber.backtrace_cleaner.dup
    ActionDispatch::LogSubscriber.backtrace_cleaner.add_silencer { |location| !location.include?(__FILE__) }
    ActionDispatch.verbose_redirect_logs = true

    draw do
      get "redirect", to: redirect("/login")
    end

    get "/redirect"
    wait

    assert_equal 3, logs.size
    assert_match(/↳ #{__FILE__}/, logs[1])
  ensure
    ActionDispatch.verbose_redirect_logs = false
    ActionDispatch::LogSubscriber.backtrace_cleaner = old_cleaner
  end

  private
    def draw(&block)
      self.class.stub_controllers do |routes|
        routes.default_url_options = { host: "www.example.com" }
        routes.draw(&block)
        @app = RoutedRackApp.new routes
      end
    end

    def get(path, **options)
      super(path, **options.merge(headers: { "action_dispatch.routes" => @app.routes }))
    end

    def logs
      @logs ||= @logger.logged(:info)
    end
end
Repository: rails/rails
File: actionpack/lib/action_dispatch/log_subscriber.rb (Ruby)

Code before:
# frozen_string_literal: true

# :markup: markdown

module ActionDispatch
  class LogSubscriber < ActiveSupport::LogSubscriber
    def redirect(event)
      payload = event.payload

      info { "Redirected to #{payload[:location]}" }

      if ActionDispatch.verbose_redirect_logs
        request = payload[:request]

        request.routes.router.recognize(request) do |route|
          info { "↳ #{route.source_location}" }
        end
      end

      info do
        status = payload[:status]

        message = +"Completed #{status} #{Rack::Utils::HTTP_STATUS_CODES[status]} in #{event.duration.round}ms"
        message << "\n\n" if defined?(Rails.env) && Rails.env.development?

        message
      end
    end
    subscribe_log_level :redirect, :info
  end
end

ActionDispatch::LogSubscriber.attach_to :action_dispatch


Review comment (suggestion):
If the log level is higher than `:info`  we can skip some code execution by wrapping it all in the `info` block.
```suggestion
      info do
        if ActionDispatch.verbose_redirect_logs
          request = payload[:request]

          request.routes.router.recognize(request) do |route|
            "↳ #{route.source_location}"
            # Or maybe we need to assign it to a instance variable here:
            # message = "↳ #{route.source_location}"
          end
          # message # maybe?
        end
      end

```

Code after:
# frozen_string_literal: true

module ActionDispatch
  class LogSubscriber < ActiveSupport::LogSubscriber # :nodoc:
    class_attribute :backtrace_cleaner, default: ActiveSupport::BacktraceCleaner.new

    def redirect(event)
      payload = event.payload

      info { "Redirected to #{payload[:location]}" }

      info do
        if ActionDispatch.verbose_redirect_logs && (source = redirect_source_location)
          "↳ #{source}"
        end
      end

      info do
        status = payload[:status]

        message = +"Completed #{status} #{Rack::Utils::HTTP_STATUS_CODES[status]} in #{event.duration.round}ms"
        message << "\n\n" if defined?(Rails.env) && Rails.env.development?

        message
      end
    end
    subscribe_log_level :redirect, :info

    private
      def redirect_source_location
        backtrace_cleaner.first_clean_frame
      end
  end
end

ActionDispatch::LogSubscriber.attach_to :action_dispatch
Repository: rails/rails
File: activesupport/lib/active_support/benchmark.rb (Ruby)

Code before:
# frozen_string_literal: true

module ActiveSupport
  # = \Benchmark
  module Benchmark
    # Benchmark realtime in seconds.
    #
    #   ActiveSupport::Benchmark.realtime { User.expensive_query.count }
    #   # => 5.8273
    def self.realtime(&block)
      time_start = Process.clock_gettime(Process::CLOCK_MONOTONIC)
      yield
      Process.clock_gettime(Process::CLOCK_MONOTONIC) - time_start
    end

    # Benchmark realtime in milliseconds.
    #
    #   ActiveSupport::Benchmark.realtime { User.all }
    #   # => 8.0e-05
    #
    #   ActiveSupport::Benchmark.ms { User.all }
    #   # => 0.074
    def self.ms(&block)
      1000 * realtime(&block)
    end
  end
end


Review comment (suggestion):
Is there a need for this method? `::Benchmark.realtime` is exactly the same: https://github.com/ruby/benchmark/blob/master/lib/benchmark.rb#L311.

Code after:
# frozen_string_literal: true

module ActiveSupport
  module Benchmark # :nodoc:
    # Benchmark realtime in the specified time unit. By default,
    # the returned unit is in seconds.
    #
    #   ActiveSupport::Benchmark.realtime { sleep 0.1 }
    #   # => 0.10007
    #
    #   ActiveSupport::Benchmark.realtime(:float_millisecond) { sleep 0.1 }
    #   # => 100.07
    #
    # `unit` can be any of the values accepted by Ruby's `Process.clock_gettime`.
    def self.realtime(unit = :float_second, &block)
      time_start = Process.clock_gettime(Process::CLOCK_MONOTONIC, unit)
      yield
      Process.clock_gettime(Process::CLOCK_MONOTONIC, unit) - time_start
    end
  end
end
Repository: rails/rails
File: activesupport/test/benchmark_test.rb (Ruby)

Code before:
# frozen_string_literal: true

require_relative "abstract_unit"

class BenchmarkTest < ActiveSupport::TestCase
  def test_realtime
    time = ActiveSupport::Benchmark.realtime { sleep 0.1 }
    assert_in_delta 0.1, time, 0.0005
  end

  def test_ms
    ms = ActiveSupport::Benchmark.realtime { sleep 0.1 }
    assert_in_delta 0.1, ms, 0.5
  end
end


Review comment (suggestion):
```suggestion
    ms = ActiveSupport::Benchmark.ms { sleep 0.1 }
```

Code after:
# frozen_string_literal: true

require_relative "abstract_unit"

class BenchmarkTest < ActiveSupport::TestCase
  def test_realtime
    time = ActiveSupport::Benchmark.realtime { sleep 0.1 }
    assert_in_delta 0.1, time, 0.0005
  end

  def test_realtime_millisecond
    ms = ActiveSupport::Benchmark.realtime(:float_millisecond) { sleep 0.1 }
    assert_in_delta 100, ms, 0.5
  end
end