ronantakizawa/codereview-bench huggingface dataset:
Repository: redis/redis-py
File: tests/test_retry.py (Python)

Code before:
@pytest.mark.parametrize(
    "args",
    [
        (ConstantBackoff(0), 0),
        (ConstantBackoff(10), 5),
        (NoBackoff(), 0),
    ]
    + [
        backoff
        for Backoff in (
            DecorrelatedJitterBackoff,
            EqualJitterBackoff,
            ExponentialBackoff,
            ExponentialWithJitterBackoff,
            FullJitterBackoff,
        )
        for backoff in ((Backoff(), 2), (Backoff(25), 5), (Backoff(25, 5), 5))
    ],
)
def test_retry_eq_and_hashable(args):
    assert Retry(*args) == Retry(*args)

    # create another retry object with different parameters
    copy = list(args)
    if copy[0] == NoBackoff:
        copy[1] == 9000
    else:
        copy[0] = NoBackoff

    assert Retry(*args) != Retry(*copy)
    assert Retry(*copy) != Retry(*args)
    assert len({Retry(*args), Retry(*args), Retry(*copy), Retry(*copy)}) == 2


class TestRetry:
    "Test that Retry calls backoff and retries the expected number of times"

    def setup_method(self, test_method):
        self.actual_attempts = 0
        self.actual_failures = 0

    def _do(self):
        self.actual_attempts += 1
        raise ConnectionError()

    def _fail(self, error):
        self.actual_failures += 1

    def _fail_inf(self, error):
        self.actual_failures += 1
        if self.actual_failures == 5:

Review comment (suggestion):
The use of '==' on line 117 does not assign the value 9000 to copy[1] as intended. Replace '==' with '=' to correctly modify the test parameter.
```suggestion
        copy[1] = 9000
```

Code after:
@pytest.mark.parametrize(
    "args",
    [
        (ConstantBackoff(0), 0),
        (ConstantBackoff(10), 5),
        (NoBackoff(), 0),
    ]
    + [
        backoff
        for Backoff in (
            DecorrelatedJitterBackoff,
            EqualJitterBackoff,
            ExponentialBackoff,
            ExponentialWithJitterBackoff,
            FullJitterBackoff,
        )
        for backoff in ((Backoff(), 2), (Backoff(25), 5), (Backoff(25, 5), 5))
    ],
)
def test_retry_eq_and_hashable(args):
    assert Retry(*args) == Retry(*args)

    # create another retry object with different parameters
    copy = list(args)
    if isinstance(copy[0], ConstantBackoff):
        copy[1] = 9000
    else:
        copy[0] = ConstantBackoff(9000)

    assert Retry(*args) != Retry(*copy)
    assert Retry(*copy) != Retry(*args)
    assert len({Retry(*args), Retry(*args), Retry(*copy), Retry(*copy)}) == 2


class TestRetry:
    "Test that Retry calls backoff and retries the expected number of times"

    def setup_method(self, test_method):
        self.actual_attempts = 0
        self.actual_failures = 0

    def _do(self):
        self.actual_attempts += 1
        raise ConnectionError()

    def _fail(self, error):
        self.actual_failures += 1

    def _fail_inf(self, error):
        self.actual_failures += 1
        if self.actual_failures == 5:
Repository: redis/redis-py
File: tests/test_retry.py (Python)

Code before:
    [
        (ConstantBackoff(0), 0),
        (ConstantBackoff(10), 5),
        (NoBackoff(), 0),
    ]
    + [
        backoff
        for Backoff in (
            DecorrelatedJitterBackoff,
            EqualJitterBackoff,
            ExponentialBackoff,
            ExponentialWithJitterBackoff,
            FullJitterBackoff,
        )
        for backoff in ((Backoff(), 2), (Backoff(25), 5), (Backoff(25, 5), 5))
    ],
)
def test_retry_eq_and_hashable(args):
    assert Retry(*args) == Retry(*args)

    # create another retry object with different parameters
    copy = list(args)
    if copy[0] == NoBackoff:
        copy[1] == 9000
    else:
        copy[0] = NoBackoff

    assert Retry(*args) != Retry(*copy)
    assert Retry(*copy) != Retry(*args)
    assert len({Retry(*args), Retry(*args), Retry(*copy), Retry(*copy)}) == 2


class TestRetry:
    "Test that Retry calls backoff and retries the expected number of times"

    def setup_method(self, test_method):
        self.actual_attempts = 0
        self.actual_failures = 0

    def _do(self):
        self.actual_attempts += 1
        raise ConnectionError()

    def _fail(self, error):
        self.actual_failures += 1

    def _fail_inf(self, error):
        self.actual_failures += 1
        if self.actual_failures == 5:
            raise ConnectionError()


Review comment (bug):
Comparing copy[0] to the NoBackoff class using '==' may be incorrect if the intent is to check for a NoBackoff instance. Consider using 'if isinstance(copy[0], NoBackoff):' for type checking.
```suggestion
    if isinstance(copy[0], NoBackoff):
        copy[1] = 9000
    else:
        copy[0] = NoBackoff()
```

Code after:
    [
        (ConstantBackoff(0), 0),
        (ConstantBackoff(10), 5),
        (NoBackoff(), 0),
    ]
    + [
        backoff
        for Backoff in (
            DecorrelatedJitterBackoff,
            EqualJitterBackoff,
            ExponentialBackoff,
            ExponentialWithJitterBackoff,
            FullJitterBackoff,
        )
        for backoff in ((Backoff(), 2), (Backoff(25), 5), (Backoff(25, 5), 5))
    ],
)
def test_retry_eq_and_hashable(args):
    assert Retry(*args) == Retry(*args)

    # create another retry object with different parameters
    copy = list(args)
    if isinstance(copy[0], ConstantBackoff):
        copy[1] = 9000
    else:
        copy[0] = ConstantBackoff(9000)

    assert Retry(*args) != Retry(*copy)
    assert Retry(*copy) != Retry(*args)
    assert len({Retry(*args), Retry(*args), Retry(*copy), Retry(*copy)}) == 2


class TestRetry:
    "Test that Retry calls backoff and retries the expected number of times"

    def setup_method(self, test_method):
        self.actual_attempts = 0
        self.actual_failures = 0

    def _do(self):
        self.actual_attempts += 1
        raise ConnectionError()

    def _fail(self, error):
        self.actual_failures += 1

    def _fail_inf(self, error):
        self.actual_failures += 1
        if self.actual_failures == 5:
            raise ConnectionError()
Repository: redis/redis-py
File: redis/utils.py (Python)

Code before:
import datetime
import logging
import textwrap
from contextlib import contextmanager
from functools import wraps
from typing import Any, Dict, List, Mapping, Optional, Union

from redis.exceptions import DataError
from redis.typing import AbsExpiryT, EncodableT, ExpiryT

try:
    import hiredis  # noqa

    # Only support Hiredis >= 3.0:
    hiredis_version = hiredis.__version__.split(".")
    HIREDIS_AVAILABLE = int(hiredis_version[0]) >= 3 and int(hiredis_version[1]) >= 2
    if not HIREDIS_AVAILABLE:
        raise ImportError("hiredis package should be >= 3.2.0")
except ImportError:
    HIREDIS_AVAILABLE = False

try:
    import ssl  # noqa

    SSL_AVAILABLE = True
except ImportError:
    SSL_AVAILABLE = False

try:
    import cryptography  # noqa

    CRYPTOGRAPHY_AVAILABLE = True
except ImportError:
    CRYPTOGRAPHY_AVAILABLE = False

from importlib import metadata


def from_url(url, **kwargs):
    """
    Returns an active Redis client generated from the given database URL.

Review comment (bug):
The version check logic may incorrectly exclude valid versions (e.g., '4.0.x'). Consider revising the condition to allow any major version above 3 or (major equal to 3 and minor at least 2).
```suggestion
    HIREDIS_AVAILABLE = (
        int(hiredis_version[0]) > 3 or
        (int(hiredis_version[0]) == 3 and int(hiredis_version[1]) >= 2)
    )
```

Code after:
import datetime
import logging
import textwrap
from contextlib import contextmanager
from functools import wraps
from typing import Any, Dict, List, Mapping, Optional, Union

from redis.exceptions import DataError
from redis.typing import AbsExpiryT, EncodableT, ExpiryT

try:
    import hiredis  # noqa

    # Only support Hiredis >= 3.0:
    hiredis_version = hiredis.__version__.split(".")
    HIREDIS_AVAILABLE = int(hiredis_version[0]) > 3 or (
        int(hiredis_version[0]) == 3 and int(hiredis_version[1]) >= 2
    )
    if not HIREDIS_AVAILABLE:
        raise ImportError("hiredis package should be >= 3.2.0")
except ImportError:
    HIREDIS_AVAILABLE = False

try:
    import ssl  # noqa

    SSL_AVAILABLE = True
except ImportError:
    SSL_AVAILABLE = False

try:
    import cryptography  # noqa

    CRYPTOGRAPHY_AVAILABLE = True
except ImportError:
    CRYPTOGRAPHY_AVAILABLE = False

from importlib import metadata


def from_url(url, **kwargs):
Repository: redis/redis-py
File: redis/commands/search/aggregation.py (Python)

Code before:
    def alias(self, alias: str) -> "Reducer":
        """
        Set the alias for this reducer.

        ### Parameters

        - **alias**: The value of the alias for this reducer. If this is the
            special value `aggregation.FIELDNAME` then this reducer will be
            aliased using the same name as the field upon which it operates.
            Note that using `FIELDNAME` is only possible on reducers which
            operate on a single field value.

        This method returns the `Reducer` object making it suitable for
        chaining.
        """
        if alias is FIELDNAME:
            if not self._field:
                raise ValueError("Cannot use FIELDNAME alias with no field")
            else:
                # Chop off initial '@'
                alias = self._field[1:]
        self._alias = alias
        return self

    @property
    def args(self) -> tuple[str, ...]:
        return self._args


class SortDirection:
    """
    This special class is used to indicate sort direction.
    """

    DIRSTRING: Optional[str] = None

    def __init__(self, field: str) -> None:
        self.field = field


class Asc(SortDirection):
    """
    Indicate that the given field should be sorted in ascending order
    """

    DIRSTRING = "ASC"


class Desc(SortDirection):
    """
    Indicate that the given field should be sorted in descending order

Review comment (suggestion):
Use `Tuple[str, ...]` instead of `tuple[str, ...]` for consistency with other type annotations in the codebase that use the typing module imports.

Code after:
    def alias(self, alias: str) -> "Reducer":
        """
        Set the alias for this reducer.

        ### Parameters

        - **alias**: The value of the alias for this reducer. If this is the
            special value `aggregation.FIELDNAME` then this reducer will be
            aliased using the same name as the field upon which it operates.
            Note that using `FIELDNAME` is only possible on reducers which
            operate on a single field value.

        This method returns the `Reducer` object making it suitable for
        chaining.
        """
        if alias is FIELDNAME:
            if not self._field:
                raise ValueError("Cannot use FIELDNAME alias with no field")
            else:
                # Chop off initial '@'
                alias = self._field[1:]
        self._alias = alias
        return self

    @property
    def args(self) -> Tuple[str, ...]:
        return self._args


class SortDirection:
    """
    This special class is used to indicate sort direction.
    """

    DIRSTRING: Optional[str] = None

    def __init__(self, field: str) -> None:
        self.field = field


class Asc(SortDirection):
    """
    Indicate that the given field should be sorted in ascending order
    """

    DIRSTRING = "ASC"


class Desc(SortDirection):
    """
    Indicate that the given field should be sorted in descending order
Repository: redis/redis-py
File: redis/retry.py (Python)

Code before:
import socket
from time import sleep
from typing import TYPE_CHECKING, Any, Callable, Iterable, Tuple, Type, TypeVar, Union

from redis.exceptions import ConnectionError, TimeoutError

T = TypeVar("T")

if TYPE_CHECKING:
    from redis.backoff import AbstractBackoff


class AbstractRetry:
    """Retry a specific number of times after a failure"""

    __slots__ = "_backoff", "_retries", "_supported_errors"
    _supported_errors: Tuple[Type[Exception], ...]

    def __init__(
        self,
        backoff: "AbstractBackoff",
        retries: int,
        supported_errors: Union[Tuple[Type[Exception], ...], None] = None,
    ):
        """
        Initialize a `Retry` object with a `Backoff` object
        that retries a maximum of `retries` times.
        `retries` can be negative to retry forever.
        You can specify the types of supported errors which trigger
        a retry with the `supported_errors` parameter.
        """
        self._backoff = backoff
        self._retries = retries
        if supported_errors:
            self._supported_errors = supported_errors

    def __eq__(self, other: Any) -> bool:
        if not isinstance(other, AbstractRetry):
            return NotImplemented

        return (

Review comment (suggestion):
I think that we don't need to use `__slots__` for the `retry` object. We are not keeping that many instances of it, and the saved memory is not going to be significant. I see that we've had both patterns in the `sync` and `async` versions, and I suggest sticking to the one that has been used by the sync `Retry` class.

Code after:
import abc
import socket
from time import sleep
from typing import TYPE_CHECKING, Any, Callable, Generic, Iterable, Tuple, Type, TypeVar

from redis.exceptions import ConnectionError, TimeoutError

T = TypeVar("T")
E = TypeVar("E", bound=Exception, covariant=True)

if TYPE_CHECKING:
    from redis.backoff import AbstractBackoff


class AbstractRetry(Generic[E], abc.ABC):
    """Retry a specific number of times after a failure"""

    _supported_errors: Tuple[Type[E], ...]

    def __init__(
        self,
        backoff: "AbstractBackoff",
        retries: int,
        supported_errors: Tuple[Type[E], ...],
    ):
        """
        Initialize a `Retry` object with a `Backoff` object
        that retries a maximum of `retries` times.
        `retries` can be negative to retry forever.
        You can specify the types of supported errors which trigger
        a retry with the `supported_errors` parameter.
        """
        self._backoff = backoff
        self._retries = retries
        self._supported_errors = supported_errors

    @abc.abstractmethod
    def __eq__(self, other: Any) -> bool:
        return NotImplemented

    def __hash__(self) -> int:
Repository: redis/redis-py
File: redis/multidb/circuit.py (Python)

Code before:
            State.HALF_OPEN: self._cb.half_open,
        }
        self._database = None

    @property
    def grace_period(self) -> float:
        return self._cb.reset_timeout

    @grace_period.setter
    def grace_period(self, grace_period: float):
        self._cb.reset_timeout = grace_period

    @property
    def state(self) -> State:
        return State(value=self._cb.state.name)

    @state.setter
    def state(self, state: State):
        self._state_pb_mapper[state]()

    @property
    def database(self):
        return self._database

    @database.setter
    def database(self, database):
        self._database = database

    def on_state_changed(self, cb: Callable[["CircuitBreaker", State, State], None]):
        listener = PBListener(cb, self.database)
        self._cb.add_listener(listener)

Review comment (suggestion):
Is there a specific reason to use multiple inheritance here? If `SyncCircuitBreaker` extends `BaseCircuitBreaker`, you could inherit from just one class, which would make it easier to follow.


Code after:
class PBListener(pybreaker.CircuitBreakerListener):
    """Wrapper for callback to be compatible with pybreaker implementation."""
    def __init__(
            self,
            cb: Callable[[SyncCircuitBreaker, State, State], None],
            database,
    ):
        """
        Initialize a PBListener instance.

        Args:
            cb: Callback function that will be called when the circuit breaker state changes.
            database: Database instance associated with this circuit breaker.
        """

        self._cb = cb
        self._database = database

    def state_change(self, cb, old_state, new_state):
        cb = PBCircuitBreakerAdapter(cb)
        cb.database = self._database
        old_state = State(value=old_state.name)
        new_state = State(value=new_state.name)
        self._cb(cb, old_state, new_state)

class PBCircuitBreakerAdapter(SyncCircuitBreaker, BaseCircuitBreaker):
    def __init__(self, cb: pybreaker.CircuitBreaker):
        """
        Initialize a PBCircuitBreakerAdapter instance.

        This adapter wraps pybreaker's CircuitBreaker implementation to make it compatible
        with our CircuitBreaker interface.

        Args:
            cb: A pybreaker CircuitBreaker instance to be adapted.
        """
        super().__init__(cb)

    def on_state_changed(self, cb: Callable[["SyncCircuitBreaker", State, State], None]):
        listener = PBListener(cb, self.database)
        self._cb.add_listener(listener)
Repository: redis/redis-py
File: tests/test_connection_pool.py (Python)

Code before:
import os
import re
import time
from contextlib import closing
from threading import Thread
from unittest import mock

from mock import Mock
import pytest
import redis
from redis.cache import CacheConfig
from redis.connection import CacheProxyConnection, Connection, to_bool
from redis.utils import SSL_AVAILABLE

from .conftest import (
    _get_client,
    skip_if_redis_enterprise,
    skip_if_resp_version,
    skip_if_server_version_lt,
)
from .test_pubsub import wait_for_message

if SSL_AVAILABLE:
    import ssl


class DummyConnection:
    description_format = "DummyConnection<>"

    def __init__(self, **kwargs):
        self.kwargs = kwargs
        self.pid = os.getpid()
        self._sock = None

Review comment (suggestion):
Use `unittest.mock.Mock` instead of importing from the `mock` package. The `mock` module was integrated into the standard library as `unittest.mock` in Python 3.3+. This import is redundant since `unittest.mock` is already imported on line 6.
```suggestion

```

Code after:
import os
import re
import time
from contextlib import closing
from threading import Thread
from unittest import mock

import pytest
import redis
from redis.cache import CacheConfig
from redis.connection import CacheProxyConnection, Connection, to_bool
from redis.utils import SSL_AVAILABLE

from .conftest import (
    _get_client,
    skip_if_redis_enterprise,
    skip_if_resp_version,
    skip_if_server_version_lt,
)
from .test_pubsub import wait_for_message

if SSL_AVAILABLE:
    import ssl


class DummyConnection:
    description_format = "DummyConnection<>"

    def __init__(self, **kwargs):
        self.kwargs = kwargs
        self.pid = os.getpid()
        self._sock = None
Repository: redis/redis-py
File: redis/commands/cluster.py (Python)

Code before:
        # Geo Operations
        "GEODIST",
        "GEOHASH",
        "GEOPOS",
        "GEOSEARCH",
        # Hash Operations
        "HEXISTS",
        "HGET",
        "HGETALL",
        "HKEYS",
        "HLEN",
        "HMGET",
        "HSTRLEN",
        "HVALS",
        "HRANDFIELD",
        "HEXPIRETIME",
        "HPEXPIRETIME",
        "HTTL",
        "HPTTL",
        "HSCAN",
        # List Operations
        "LINDEX",
        "LPOS",
        "LLEN",
        "LRANGE",
        "LCS",
        # Set Operations
        "SCARD",
        "SDIFF",
        "SINTER",
        "SINTERCARD",
        "SISMEMBER",
        "SMISMEMBER",
        "SMEMBERS",
        "SRANDMEMBER",
        "SUNION",
        "SSCAN",
        # Sorted Set Operations
        "ZCARD",
        "ZCOUNT",
        "ZDIFF",
        "ZINTER",
        "ZINTERCARD",
        "ZLEXCOUNT",
        "ZMSCORE",
        "ZRANDMEMBER",
        "ZRANGE",
        "ZRANGEBYLEX",
        "ZRANGEBYSCORE",
        "ZRANK",
        "ZREVRANGE",

Review comment (suggestion):
LCS (Longest Common Subsequence) is a string operation command that compares two string keys, not a list operation. According to the Redis documentation and the implementation in `BasicKeyCommands` class (not `ListCommands`), it should be categorized under "String Operations" instead of "List Operations".

Suggestion: Move LCS from line 106 to the String Operations section (after line 80).

Code after:
        "LCS",
        # Geo Operations
        "GEODIST",
        "GEOHASH",
        "GEOPOS",
        "GEOSEARCH",
        # Hash Operations
        "HEXISTS",
        "HGET",
        "HGETALL",
        "HKEYS",
        "HLEN",
        "HMGET",
        "HSTRLEN",
        "HVALS",
        "HRANDFIELD",
        "HEXPIRETIME",
        "HPEXPIRETIME",
        "HTTL",
        "HPTTL",
        "HSCAN",
        # List Operations
        "LINDEX",
        "LPOS",
        "LLEN",
        "LRANGE",
        # Set Operations
        "SCARD",
        "SDIFF",
        "SINTER",
        "SINTERCARD",
        "SISMEMBER",
        "SMISMEMBER",
        "SMEMBERS",
        "SRANDMEMBER",
        "SUNION",
        "SSCAN",
        # Sorted Set Operations
        "ZCARD",
        "ZCOUNT",
        "ZDIFF",
        "ZINTER",
        "ZINTERCARD",
        "ZLEXCOUNT",
        "ZMSCORE",
        "ZRANDMEMBER",
        "ZRANGE",
        "ZRANGEBYLEX",
        "ZRANGEBYSCORE",
        "ZRANK",
        "ZREVRANGE",
Repository: redis/redis-py
File: tests/test_asyncio/test_connection_pool.py (Python)

Code before:
import asyncio
import re
from unittest import mock
from unittest.mock import patch

import pytest
import pytest_asyncio
import redis.asyncio as redis
from redis.asyncio.connection import (
    BlockingConnectionPool,
    Connection,
    ConnectionPool,
    to_bool,
)
from redis.auth.token import TokenInterface
from tests.conftest import skip_if_redis_enterprise, skip_if_server_version_lt

from .compat import aclosing
from .conftest import asynccontextmanager
from .test_pubsub import wait_for_message


@pytest.mark.onlynoncluster
class TestRedisAutoReleaseConnectionPool:
    @pytest_asyncio.fixture
    async def r(self, create_redis) -> redis.Redis:
        """This is necessary since r and r2 create ConnectionPools behind the scenes"""
        r = await create_redis()
        r.auto_close_connection_pool = True
        yield r

    @staticmethod
    def get_total_connected_connections(pool):
        return len(pool._available_connections) + len(pool._in_use_connections)

    @staticmethod
    async def create_two_conn(r: redis.Redis):
        if not r.single_connection_client:  # Single already initialized connection
            r.connection = await r.connection_pool.get_connection()
        return await r.connection_pool.get_connection()

Review comment (suggestion):
The imports from unittest.mock are split across two lines (line 4 and line 15). These should be combined into a single import statement for consistency and cleaner code organization.

Code after:
import asyncio
import re
from unittest import mock
from unittest.mock import patch

import pytest
import pytest_asyncio
import redis.asyncio as redis
from redis.asyncio.connection import (
    BlockingConnectionPool,
    Connection,
    ConnectionPool,
    to_bool,
)
from unittest.mock import AsyncMock, MagicMock

from redis.observability.metrics import CloseReason
from redis.auth.token import TokenInterface
from tests.conftest import skip_if_redis_enterprise, skip_if_server_version_lt

from .compat import aclosing
from .conftest import asynccontextmanager
from .test_pubsub import wait_for_message


@pytest.mark.onlynoncluster
class TestRedisAutoReleaseConnectionPool:
    @pytest_asyncio.fixture
    async def r(self, create_redis) -> redis.Redis:
        """This is necessary since r and r2 create ConnectionPools behind the scenes"""
        r = await create_redis()
        r.auto_close_connection_pool = True
        yield r

    @staticmethod
    def get_total_connected_connections(pool):
        return len(pool._available_connections) + len(pool._in_use_connections)

    @staticmethod
    async def create_two_conn(r: redis.Redis):