ronantakizawa/codereview-bench huggingface dataset:
Repository: psf/black
File: tests/data/cases/context_managers_39.py (Python)

Code before:

async def func():
    async with \
        make_context_manager1() as cm1, \
        make_context_manager2() as cm2, \
        make_context_manager3() as cm3, \
        make_context_manager4() as cm4 \
    :
        pass

    async with some_function(
        argument1, argument2, argument3="some_value"
    ) as some_cm, some_other_function(
        argument1, argument2, argument3="some_value"
    ):
        pass



# don't remove the brackets here, it changes the meaning of the code.
with (x, y) as z:
    pass


# don't remove the brackets here, it changes the meaning of the code.
# even tough the code will always trigger a runtime error
with (name_5, name_4), name_5:
    pass


def test_tuple_as_contextmanager():
    from contextlib import nullcontext

    try:
        with (nullcontext(),nullcontext()),nullcontext():
            pass
    except TypeError: 
        # test passed
        pass
    else:
        # this should be a type error
        assert False

# output


with (
    make_context_manager1() as cm1,
    make_context_manager2() as cm2,
    make_context_manager3() as cm3,
    make_context_manager4() as cm4,

Review comment (suggestion):
```suggestion
# even though the code will always trigger a runtime error
```

Code after:

async def func():
    async with \
        make_context_manager1() as cm1, \
        make_context_manager2() as cm2, \
        make_context_manager3() as cm3, \
        make_context_manager4() as cm4 \
    :
        pass

    async with some_function(
        argument1, argument2, argument3="some_value"
    ) as some_cm, some_other_function(
        argument1, argument2, argument3="some_value"
    ):
        pass



# don't remove the brackets here, it changes the meaning of the code.
with (x, y) as z:
    pass


# don't remove the brackets here, it changes the meaning of the code.
# even though the code will always trigger a runtime error
with (name_5, name_4), name_5:
    pass


def test_tuple_as_contextmanager():
    from contextlib import nullcontext

    try:
        with (nullcontext(),nullcontext()),nullcontext():
            pass
    except TypeError: 
        # test passed
        pass
    else:
        # this should be a type error
        assert False

# output


with (
    make_context_manager1() as cm1,
    make_context_manager2() as cm2,
    make_context_manager3() as cm3,
    make_context_manager4() as cm4,
Repository: psf/black
File: tests/data/cases/context_managers_39.py (Python)

Code before:
    pass


async def func():
    async with (
        make_context_manager1() as cm1,
        make_context_manager2() as cm2,
        make_context_manager3() as cm3,
        make_context_manager4() as cm4,
    ):
        pass

    async with (
        some_function(argument1, argument2, argument3="some_value") as some_cm,
        some_other_function(argument1, argument2, argument3="some_value"),
    ):
        pass


# don't remove the brackets here, it changes the meaning of the code.
with (x, y) as z:
    pass


# don't remove the brackets here, it changes the meaning of the code.
# even tough the code will always trigger a runtime error
with (name_5, name_4), name_5:
    pass


def test_tuple_as_contextmanager():
    from contextlib import nullcontext

    try:
        with (nullcontext(), nullcontext()), nullcontext():
            pass
    except TypeError:
        # test passed
        pass
    else:
        # this should be a type error
        assert False


Review comment (suggestion):
```suggestion
# even though the code will always trigger a runtime error
```

Code after:
    pass


async def func():
    async with (
        make_context_manager1() as cm1,
        make_context_manager2() as cm2,
        make_context_manager3() as cm3,
        make_context_manager4() as cm4,
    ):
        pass

    async with (
        some_function(argument1, argument2, argument3="some_value") as some_cm,
        some_other_function(argument1, argument2, argument3="some_value"),
    ):
        pass


# don't remove the brackets here, it changes the meaning of the code.
with (x, y) as z:
    pass


# don't remove the brackets here, it changes the meaning of the code.
# even though the code will always trigger a runtime error
with (name_5, name_4), name_5:
    pass


def test_tuple_as_contextmanager():
    from contextlib import nullcontext

    try:
        with (nullcontext(), nullcontext()), nullcontext():
            pass
    except TypeError:
        # test passed
        pass
    else:
        # this should be a type error
        assert False
Repository: psf/black
File: src/black/__init__.py (Python)

Code before:
        exists=True, file_okay=True, dir_okay=True, readable=True, allow_dash=True
    ),
    is_eager=True,
    metavar="SRC ...",
)
@click.option(
    "--config",
    type=click.Path(
        exists=True,
        file_okay=True,
        dir_okay=False,
        readable=True,
        allow_dash=False,
        path_type=str,
    ),
    is_eager=True,
    callback=read_pyproject_toml,
    help="Read configuration options from a configuration file.",
)
@click.option(
    "--no-cache",
    is_flag=True,
    help=(
        "Do not use the cache when formatting files. This forces Black to reformat all"
        " files and not skip any unchanged file Also prevents Black from updating the"
        " cache after formatting."
    ),
)
@click.pass_context
def main(  # noqa: C901
    ctx: click.Context,
    code: Optional[str],
    line_length: int,
    target_version: list[TargetVersion],
    check: bool,
    diff: bool,
    line_ranges: Sequence[str],
    color: bool,
    fast: bool,
    pyi: bool,
    ipynb: bool,
    python_cell_magics: Sequence[str],
    skip_source_first_line: bool,
    skip_string_normalization: bool,
    skip_magic_trailing_comma: bool,
    preview: bool,
    unstable: bool,
    enable_unstable_feature: list[Preview],
    quiet: bool,
    verbose: bool,
    required_version: Optional[str],

Review comment (performance):
Nitpick
```suggestion
        "Skip reading and writing the cache, forcing Black to reformat all"
         " included files."
```

Code after:
        exists=True, file_okay=True, dir_okay=True, readable=True, allow_dash=True
    ),
    is_eager=True,
    metavar="SRC ...",
)
@click.option(
    "--config",
    type=click.Path(
        exists=True,
        file_okay=True,
        dir_okay=False,
        readable=True,
        allow_dash=False,
        path_type=str,
    ),
    is_eager=True,
    callback=read_pyproject_toml,
    help="Read configuration options from a configuration file.",
)
@click.option(
    "--no-cache",
    is_flag=True,
    help=(
        "Skip reading and writing the cache, forcing Black to reformat all"
        " included files."
    ),
)
@click.pass_context
def main(  # noqa: C901
    ctx: click.Context,
    code: Optional[str],
    line_length: int,
    target_version: list[TargetVersion],
    check: bool,
    diff: bool,
    line_ranges: Sequence[str],
    color: bool,
    fast: bool,
    pyi: bool,
    ipynb: bool,
    python_cell_magics: Sequence[str],
    skip_source_first_line: bool,
    skip_string_normalization: bool,
    skip_magic_trailing_comma: bool,
    preview: bool,
    unstable: bool,
    enable_unstable_feature: list[Preview],
    quiet: bool,
    verbose: bool,
    required_version: Optional[str],
    include: Pattern[str],
Repository: psf/black
File: tests/data/cases/jupytext_markdown_fmt.py (Python)

Code before:
# output

# Test that Jupytext markdown comments are preserved before fmt:off/on blocks
# %% [markdown]

# fmt: off
# fmt: on

# Also test with other comments
# Some comment
# %% [markdown]
# Another comment

# fmt: off
x = 1
# fmt: on

# Test multiple markdown comments
# %% [markdown]
# First markdown
# %% [code]
# Code cell

# fmt: off
y = 2
# fmt: on

Review comment (suggestion):
```suggestion

```

The `output` section can be omitted to automatically assert no changes

Code after:
# Test that Jupytext markdown comments are preserved before fmt:off/on blocks
# %% [markdown]

# fmt: off
# fmt: on

# Also test with other comments
# Some comment
# %% [markdown]
# Another comment

# fmt: off
x = 1
# fmt: on

# Test multiple markdown comments
# %% [markdown]
# First markdown
# %% [code]
# Code cell

# fmt: off
y = 2
# fmt: on
Repository: puma/puma
File: test/test_config.rb (Ruby)

Code before:
# frozen_string_literal: true

require_relative "helper"
require_relative "helpers/config_file"

require "puma/configuration"
require 'puma/events'

class TestConfigFile < TestConfigFileBase
  parallelize_me!

  def test_default_max_threads
    max_threads = 16
    max_threads = 5 if RUBY_ENGINE.nil? || RUBY_ENGINE == 'ruby'
    assert_equal max_threads, Puma::Configuration.new.default_max_threads
  end

  def test_config_loads_correct_min_threads
    conf = Puma::Configuration.new
    assert_equal 0, conf.options.default_options[:min_threads]

    ENV['MIN_THREADS'] = '7'
    conf = Puma::Configuration.new
    assert_equal 7, conf.options.default_options[:min_threads]

    ENV['PUMA_MIN_THREADS'] = '8'
    conf = Puma::Configuration.new
    assert_equal 8, conf.options.default_options[:min_threads]
  end

  def test_config_loads_correct_max_threads
    conf = Puma::Configuration.new
    assert_equal conf.default_max_threads, conf.options.default_options[:max_threads]

    ENV['MAX_THREADS'] = '7'
    conf = Puma::Configuration.new
    assert_equal 7, conf.options.default_options[:max_threads]

    ENV['PUMA_MAX_THREADS'] = '8'
    conf = Puma::Configuration.new
    assert_equal 8, conf.options.default_options[:max_threads]
  end

  def test_config_loads_correct_workers
    conf = Puma::Configuration.new
    assert_equal 0, conf.options.default_options[:workers]


Review comment (suggestion):
Setting a global and running tests in parallel threads, you're gonna have a Bad Time (as you've seen on your tests).

Maybe just removing parallelize_me will fix everything here. Put these tests into a separate test class which is not paralellized.

Code after:
# frozen_string_literal: true

require_relative "helper"
require_relative "helpers/config_file"

require "puma/configuration"
require 'puma/events'


class TestConfigFile < TestConfigFileBase
  parallelize_me!

  def test_default_max_threads
    max_threads = 16
    max_threads = 5 if RUBY_ENGINE.nil? || RUBY_ENGINE == 'ruby'
    assert_equal max_threads, Puma::Configuration.new.default_max_threads
  end


  def test_app_from_rackup
    conf = Puma::Configuration.new do |c|
      c.rackup "test/rackup/hello-bind.ru"
    end
    conf.load

    # suppress deprecation warning of Rack (>= 2.2.0)
    # > Parsing options from the first comment line is deprecated!\n
    assert_output(nil, nil) do
      conf.app
    end

    assert_equal ["tcp://127.0.0.1:9292"], conf.options[:binds]
  end

  def test_app_from_app_DSL
    conf = Puma::Configuration.new do |c|
      c.load "test/config/app.rb"
    end
    conf.load

    app = conf.app

    assert_equal [200, {}, ["embedded app"]], app.call({})
  end

  def test_ssl_configuration_from_DSL
    conf = Puma::Configuration.new do |config|
Repository: puma/puma
File: lib/puma/server.rb (Ruby)

Code before:
        @status = :halt
        return true
      when RESTART_COMMAND
        @status = :restart
        return true
      end

      return false
    end

    # Runs the provided block if the Reactor queue is enabled.
    # Returns block return value if queue is enabled, or nil if queue is not enabled.
    def if_queue_requests
      @queue_mutex.synchronize do
        if @queue_requests
          yield
        end
      end
    end
    private :if_queue_requests

    # Queues the provided client
    def queue_client(client, timeout)
      if_queue_requests do
        client.set_timeout timeout
        @reactor.add client
      end
    end
    private :queue_client

    # Given a connection on +client+, handle the incoming requests.
    #
    # This method support HTTP Keep-Alive so it may, depending on if the client
    # indicates that it supports keep alive, wait for another request before
    # returning.
    #
    # Returns true if one or more complete requests were successfully handled.
    def process_client(client, buffer)
      begin

        clean_thread_locals = @options[:clean_thread_locals]
        close_socket = true

        requests = 0

        while true
          case handle_request(client, buffer).tap {requests += 1}
          when false
            return
          when :async
            close_socket = false

Review comment (refactor):
Would it be possible to keep concurrency as a concern of the Reactor rather than the Server? It sort of feels like we could refactor this a little further to keep concurrency concerns out of this class.

Code after:
              end
            end
          rescue Object => e
            @events.unknown_error e, nil, "Listen loop"
          end
        end

        @events.fire :state, @status

        if queue_requests
          @shutdown_mutex.synchronize do
            @queue_requests = false
          end
          @reactor.clear!
          @reactor.shutdown
        end
        graceful_shutdown if @status == :stop || @status == :restart
      rescue Exception => e
        @events.unknown_error e, nil, "Exception handling servers"
      ensure
        begin
          @check.close unless @check.closed?
        rescue Errno::EBADF, RuntimeError
          # RuntimeError is Ruby 2.2 issue, can't modify frozen IOError
          # Errno::EBADF is infrequently raised
        end
        @notify.close
        @notify = nil
        @check = nil
      end

      @events.fire :state, :done
    end

    # :nodoc:
    def handle_check
      cmd = @check.read(1)

      case cmd
      when STOP_COMMAND
        @status = :stop
        return true
      when HALT_COMMAND
        @status = :halt
        return true
      when RESTART_COMMAND
        @status = :restart
        return true
      end

      return false
Repository: puma/puma
File: test/test_puma_server.rb (Ruby)

Code before:
  end

  # Retryable errors such as ECONNABORTED should be silently swallowed by accept loop.
  def test_accept_econnaborted
    # Match Ruby #accept_nonblock implementation, ECONNABORTED error is extended by IO::WaitReadable.
    error = Errno::ECONNABORTED.new('accept(2) would block').tap {|e| e.extend IO::WaitReadable}
    stub_accept_nonblock(error)
    assert_empty @events.stderr.string
  end

  # see      https://github.com/puma/puma/issues/2390
  # fixed by https://github.com/puma/puma/pull/2279
  #
  def test_client_quick_close_no_lowlevel_error_handler_call
    handler = ->(err, env, status) {
      @events.stdout.write "LLEH #{err.message}"
      [500, {"Content-Type" => "application/json"}, ["{}\n"]]
    }

    @server = Puma::Server.new @app, @events, {:lowlevel_error_handler => handler}

    server_run app: ->(env) { [200, {}, ['Hello World']] }

    sock = TCPSocket.new @host, @port
    # normal request doesn't generate EOFError
    sock.syswrite "GET / HTTP"
    sock.close

    sleep 0.7 # need some delay for the lleh to be called
    assert_empty @events.stdout.string
  end
end


Review comment (suggestion):
Since this test isn't specifically testing malformed HTTP requests, it'd be nice to make sure that this one is well-formed.

```ruby
sock.syswrite "GET / HTTP/1.1\r\n\r\n"
```

Code after:
  end

  # Retryable errors such as ECONNABORTED should be silently swallowed by accept loop.
  def test_accept_econnaborted
    # Match Ruby #accept_nonblock implementation, ECONNABORTED error is extended by IO::WaitReadable.
    error = Errno::ECONNABORTED.new('accept(2) would block').tap {|e| e.extend IO::WaitReadable}
    stub_accept_nonblock(error)
    assert_empty @events.stderr.string
  end

  # see      https://github.com/puma/puma/issues/2390
  # fixed by https://github.com/puma/puma/pull/2279
  #
  def test_client_quick_close_no_lowlevel_error_handler_call
    handler = ->(err, env, status) {
      @events.stdout.write "LLEH #{err.message}"
      [500, {"Content-Type" => "application/json"}, ["{}\n"]]
    }

    @server = Puma::Server.new @app, @events, {:lowlevel_error_handler => handler}

    server_run app: ->(env) { [200, {}, ['Hello World']] }

    # valid req & read, close
    sock = TCPSocket.new @host, @port
    sock.syswrite "GET / HTTP/1.0\r\n\r\n"
    resp = sock.sysread 256
    sock.close
    assert_match 'Hello World', resp
    sleep 0.5
    assert_empty @events.stdout.string

    # valid req, close
    sock = TCPSocket.new @host, @port
    sock.syswrite "GET / HTTP/1.0\r\n\r\n"
    sock.close
    sleep 0.5
    assert_empty @events.stdout.string

    # invalid req, close
    sock = TCPSocket.new @host, @port
    sock.syswrite "GET / HTTP"
    sock.close
    sleep 0.5
    assert_empty @events.stdout.string
  end
end
Repository: puma/puma
File: lib/puma/systemd.rb (Ruby)

Code before:
      @events = events
    end

    def hook_events
      @events.on_booted { SdNotify.ready }
      @events.on_stopped { SdNotify.stopping }
      @events.on_restart { SdNotify.reloading }
    end

    def start_watchdog
      ping_f = watchdog_sleep_time
      return unless ping_f

      log "Pinging systemd watchdog every #{ping_f.round(1)} sec"
      Thread.new do
        loop do
          sleep ping_f
          SdNotify.watchdog
        end
      end
    end

    private

    def watchdog_sleep_time
      return unless SdNotify.watchdog?

      usec = Integer(ENV["WATCHDOG_USEC"])

      sec_f = usec / 1_000_000.0
      # "It is recommended that a daemon sends a keep-alive notification message
      # to the service manager every half of the time returned here."
      sec_f / 2
    end

    def log(str)
      @events.log str
    end
  end
end


Review comment (refactor):
I think it would improve the understanding here to move this up to just after the `def start_watchdog` line.

Code after:
      @events = events
    end

    def hook_events
      @events.on_booted { SdNotify.ready }
      @events.on_stopped { SdNotify.stopping }
      @events.on_restart { SdNotify.reloading }
    end

    def start_watchdog
      return unless SdNotify.watchdog?

      ping_f = watchdog_sleep_time

      log "Pinging systemd watchdog every #{ping_f.round(1)} sec"
      Thread.new do
        loop do
          sleep ping_f
          SdNotify.watchdog
        end
      end
    end

    private

    def watchdog_sleep_time
      usec = Integer(ENV["WATCHDOG_USEC"])

      sec_f = usec / 1_000_000.0
      # "It is recommended that a daemon sends a keep-alive notification message
      # to the service manager every half of the time returned here."
      sec_f / 2
    end

    def log(str)
      @events.log str
    end
  end
end
Repository: puma/puma
File: lib/puma/const.rb (Ruby)

Code before:
    TRANSFER_ENCODING2 = "HTTP_TRANSFER_ENCODING".freeze

    CONNECTION_CLOSE = "Connection: close\r\n".freeze
    CONNECTION_KEEP_ALIVE = "Connection: Keep-Alive\r\n".freeze

    TRANSFER_ENCODING_CHUNKED = "Transfer-Encoding: chunked\r\n".freeze
    CLOSE_CHUNKED = "0\r\n\r\n".freeze

    CHUNKED = "chunked".freeze

    COLON = ": ".freeze

    NEWLINE = "\n".freeze
    HTTP_INJECTION_REGEX = /[\r\n]/.freeze

    HIJACK_P = "rack.hijack?".freeze
    HIJACK = "rack.hijack".freeze
    HIJACK_IO = "rack.hijack_io".freeze

    EARLY_HINTS = "rack.early_hints".freeze

    # Mininum interval to checks worker health
    WORKER_CHECK_INTERVAL = 5

    # Illegal character in the key or value of response header
    ILLEGAL_HEADER_KEY_REGEX = /[\u0000-\u0025|#{Regexp.escape('"(),/:;<=>?@[]{}')}]/.freeze
    ILLEGAL_HEADER_VALUE_REGEX = /[\000-\037]/.freeze

    # Banned keys of response header
    BANNED_HEADER_KEY = /rack.|status/.freeze
  end
end


Review comment (security):
maybe `Regexp.escape('"(),/:;<=>?@[]{}')` can be extracted to improve readibility?
```ruby
ILLEGAL_HEADER_CHAR_REGEX = Regexp.escape('"(),/:;<=>?@[]{}').freeze
ILLEGAL_HEADER_KEY_REGEX = /[\u0000-\u0025|#{ILLEGAL_HEADER_CHAR_REGEX}]/.freeze
```

Code after:
    TRANSFER_ENCODING2 = "HTTP_TRANSFER_ENCODING".freeze

    CONNECTION_CLOSE = "Connection: close\r\n".freeze
    CONNECTION_KEEP_ALIVE = "Connection: Keep-Alive\r\n".freeze

    TRANSFER_ENCODING_CHUNKED = "Transfer-Encoding: chunked\r\n".freeze
    CLOSE_CHUNKED = "0\r\n\r\n".freeze

    CHUNKED = "chunked".freeze

    COLON = ": ".freeze

    NEWLINE = "\n".freeze

    HIJACK_P = "rack.hijack?".freeze
    HIJACK = "rack.hijack".freeze
    HIJACK_IO = "rack.hijack_io".freeze

    EARLY_HINTS = "rack.early_hints".freeze

    # Mininum interval to checks worker health
    WORKER_CHECK_INTERVAL = 5

    # Illegal character in the key or value of response header
    DQUOTE = "\"".freeze
    HTTP_HEADER_DELIMITER = Regexp.escape("(),/:;<=>?@[]{}").freeze
    ILLEGAL_HEADER_KEY_REGEX = /[\u0000-\u0025|#{DQUOTE}|#{HTTP_HEADER_DELIMITER}]/.freeze
    ILLEGAL_HEADER_VALUE_REGEX = /[\000-\037]/.freeze

    # Banned keys of response header
    BANNED_HEADER_KEY = /rack.|status/.freeze
  end
end
Repository: puma/puma
File: lib/puma/queue_close.rb (Ruby)

Code before:
# Queue#close was added in Ruby 2.3.
# Add a simple implementation for earlier Ruby versions.
unless Queue.instance_methods.include?(:close)
  class ClosedQueueError < StandardError; end
  module Puma
    module QueueClose
      def initialize
        @closed = false
        super
      end
      def close
        @closed = true
      end
      def closed?
        @closed
      end
      def push(object)
        raise ClosedQueueError if @closed
        super
      end
      alias << push
    end
    Queue.prepend QueueClose
  end
end


Review comment (suggestion):
This comment isn't really 100% related to your changes @MSP-Greg , but I'm wondering if it strikes you as weird that Puma essentially monkey patches a class provided by the stdlib.

It seems like it'd be safer for us to just subclass Queue into a new puma-specific Queue class that provided the appropriate overrides for `close`, `closed?`, and `push`.

Code after:
class ClosedQueueError < StandardError; end
module Puma

  # Queue#close was added in Ruby 2.3.
  # Add a simple implementation for earlier Ruby versions.
  #
  module QueueClose
    def initialize
      @closed = false
      super
    end
    def close
      @closed = true
    end
    def closed?
      @closed
    end
    def push(object)
      raise ClosedQueueError if @closed
      super
    end
    alias << push
  end
  ::Queue.prepend QueueClose
end