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

Code before:
            },
        )

    def test_nonexistent_alias(self):
        msg = "Could not find config for 'nonexistent' in settings.STORAGES."
        storages = StorageHandler()
        with self.assertRaisesMessage(InvalidStorageError, msg):
            storages["nonexistent"]

    def test_nonexistent_backend(self):
        test_storages = StorageHandler(
            {
                "invalid_backend": {
                    "BACKEND": "django.nonexistent.NonexistentBackend",
                },
            }
        )
        msg = (
            "Could not find backend 'django.nonexistent.NonexistentBackend': "
            "No module named 'django.nonexistent'"
        )
        with self.assertRaisesMessage(InvalidStorageError, msg):
            test_storages["invalid_backend"]


class LazyObjectHandlingTests(SimpleTestCase):
    def test_lazy_object_is_not_evaluated_before_manual_access(self):
        obj = Storage()
        self.assertIs(obj.lazy_storage.storage._wrapped, empty)
        # assertEqual triggers resolution.
        self.assertEqual(obj.lazy_storage.storage, temp_storage)


Review comment (suggestion):
Small nitpick:
```suggestion
class StorageLazyObjectTests(SimpleTestCase):
```

Code after:
            },
        )

    def test_nonexistent_alias(self):
        msg = "Could not find config for 'nonexistent' in settings.STORAGES."
        storages = StorageHandler()
        with self.assertRaisesMessage(InvalidStorageError, msg):
            storages["nonexistent"]

    def test_nonexistent_backend(self):
        test_storages = StorageHandler(
            {
                "invalid_backend": {
                    "BACKEND": "django.nonexistent.NonexistentBackend",
                },
            }
        )
        msg = (
            "Could not find backend 'django.nonexistent.NonexistentBackend': "
            "No module named 'django.nonexistent'"
        )
        with self.assertRaisesMessage(InvalidStorageError, msg):
            test_storages["invalid_backend"]


class StorageLazyObjectTests(SimpleTestCase):
    def test_lazy_object_is_not_evaluated_before_manual_access(self):
        obj = Storage()
        self.assertIs(obj.lazy_storage.storage._wrapped, empty)
        # assertEqual triggers resolution.
        self.assertEqual(obj.lazy_storage.storage, temp_storage)
Repository: django/django
File: tests/utils_tests/utils.py (Python)

Code before:
import gc
import platform
from contextlib import contextmanager


def on_macos_with_hfs():
    """
    MacOS 10.13 (High Sierra) and lower can use HFS+ as a filesystem.
    HFS+ has a time resolution of only one second which can be too low for
    some of the tests.
    """
    macos_version = platform.mac_ver()[0]
    if macos_version != "":
        parsed_macos_version = tuple(int(x) for x in macos_version.split("."))
        return parsed_macos_version < (10, 14)
    return False


@contextmanager
def gc_debug_context():
    """
    Context manager for GC debugging to detect memory leaks.
    Reusable utility for memory leak tests.
    """
    original_debug = gc.get_debug()
    was_enabled = gc.isenabled()

    gc.disable()
    gc.collect()
    gc.set_debug(gc.DEBUG_SAVEALL)

    try:
        yield
    finally:
        gc.collect()
        gc.set_debug(original_debug)
        if was_enabled:
            gc.enable()


Review comment (suggestion):
A context manager is the right approach, but I was expecting to factor out the guts of `test_release_memory_without_garbage_collection` and use it both here and there.

Code after:
import platform


def on_macos_with_hfs():
    """
    MacOS 10.13 (High Sierra) and lower can use HFS+ as a filesystem.
    HFS+ has a time resolution of only one second which can be too low for
    some of the tests.
    """
    macos_version = platform.mac_ver()[0]
    if macos_version != "":
        parsed_macos_version = tuple(int(x) for x in macos_version.split("."))
        return parsed_macos_version < (10, 14)
    return False
Repository: django/django
File: tests/gis_tests/geoapp/models.py (Python)

Code before:

    def get_attname_column(self):
        attname, column = super().get_attname_column()
        return attname, None


class NonConcreteModel(NamedModel):
    non_concrete = NonConcreteField()
    point = models.PointField(geography=True)


class ManyPointModel(NamedModel):
    point1 = models.PointField()
    point2 = models.PointField()
    point3 = models.PointField(srid=3857)


class Points(models.Model):
    geom = models.MultiPointField()


class Lines(models.Model):
    geom = models.MultiLineStringField()


class GeometryCollections(models.Model):
    geom = models.GeometryCollectionField()


Review comment (suggestion):
Should this be singular and not plural? `GeometryCollections(geom=single_collection)` doesn't seem right, although I get `Lines(geom=lines)`.

Code after:

    def get_attname_column(self):
        attname, column = super().get_attname_column()
        return attname, None


class NonConcreteModel(NamedModel):
    non_concrete = NonConcreteField()
    point = models.PointField(geography=True)


class ManyPointModel(NamedModel):
    point1 = models.PointField()
    point2 = models.PointField()
    point3 = models.PointField(srid=3857)


class Points(models.Model):
    geom = models.MultiPointField()


class Lines(models.Model):
    geom = models.MultiLineStringField()


class GeometryCollectionModel(models.Model):
    geom = models.GeometryCollectionField()
Repository: django/django
File: django/utils/http.py (Python)

Code before:

def base36_to_int(s):
    """
    Convert a base 36 string to an int. Raise ValueError if the input won't fit
    into an int.
    """
    # To prevent overconsumption of server resources, reject any
    # base36 string that is longer than 13 base36 digits (13 digits
    # is sufficient to base36-encode any 64-bit integer)
    if len(s) > 13:
        raise ValueError("Base36 input too large")
    return int(s, 36)


def int_to_base36(i):
    """Convert an integer to a base36 string."""
    char_set = "0123456789abcdefghijklmnopqrstuvwxyz"
    if i < 0:
        raise ValueError("Negative base36 conversion input.")
    if i < 36:
        return char_set[i]
    b36 = []
    while i != 0:
        i, n = divmod(i, 36)
        b36.append(char_set[n])
    return "".join(reversed(b36))


def urlsafe_base64_encode(s):
    """
    Encode a bytestring to a base64 string for use in URLs. Strip any trailing
    equal signs.
    """
    return base64.urlsafe_b64encode(s).rstrip(b"\n=").decode("ascii")


def urlsafe_base64_decode(s):
    """
    Decode a base64 encoded string. Add back any trailing equal signs that
    might have been stripped.
    """
    s = s.encode()
    try:
        return base64.urlsafe_b64decode(s.ljust(len(s) + len(s) % 4, b"="))
    except (LookupError, BinasciiError) as e:
        raise ValueError(e)


def parse_etags(etag_str):
    """
    Parse a string of ETags given in an If-None-Match or If-Match header as

Review comment (suggestion):
I get appending and finally reversing to avoid `insert()`, but in that case I think we need to update the `b36` variable name.

Code after:

def base36_to_int(s):
    """
    Convert a base 36 string to an int. Raise ValueError if the input won't fit
    into an int.
    """
    # To prevent overconsumption of server resources, reject any
    # base36 string that is longer than 13 base36 digits (13 digits
    # is sufficient to base36-encode any 64-bit integer)
    if len(s) > 13:
        raise ValueError("Base36 input too large")
    return int(s, 36)


def int_to_base36(i):
    """Convert an integer to a base36 string."""
    char_set = "0123456789abcdefghijklmnopqrstuvwxyz"
    if i < 0:
        raise ValueError("Negative base36 conversion input.")
    if i < 36:
        return char_set[i]
    b36_parts = []
    while i != 0:
        i, n = divmod(i, 36)
        b36_parts.append(char_set[n])
    return "".join(reversed(b36_parts))


def urlsafe_base64_encode(s):
    """
    Encode a bytestring to a base64 string for use in URLs. Strip any trailing
    equal signs.
    """
    return base64.urlsafe_b64encode(s).rstrip(b"\n=").decode("ascii")


def urlsafe_base64_decode(s):
    """
    Decode a base64 encoded string. Add back any trailing equal signs that
    might have been stripped.
    """
    s = s.encode()
    try:
        return base64.urlsafe_b64decode(s.ljust(len(s) + len(s) % 4, b"="))
    except (LookupError, BinasciiError) as e:
        raise ValueError(e)


def parse_etags(etag_str):
    """
    Parse a string of ETags given in an If-None-Match or If-Match header as
Repository: django/django
File: tests/model_forms/models.py (Python)

Code before:
        required_db_features = {"supports_table_check_constraints"}
        constraints = [
            models.UniqueConstraint(
                "name",
                "category",
                name="unique_name_category",
                violation_error_message="This product already exists.",
            ),
            models.CheckConstraint(
                condition=models.Q(price__gt=0),
                name="price_gte_zero",
                violation_error_message="Price must be greater than zero.",
            ),
        ]


class AttnameConstraintsModel(models.Model):
    left = models.ForeignKey(
        "self", related_name="+", null=True, on_delete=models.SET_NULL
    )
    right = models.ForeignKey(
        "self", related_name="+", null=True, on_delete=models.SET_NULL
    )

    class Meta:
        required_db_features = {"supports_table_check_constraints"}
        constraints = [
            models.CheckConstraint(
                name="%(app_label)s_%(class)s_left_not_right",
                # right_id here is the ForeignKey's attname, not name
                condition=~models.Q(left=models.F("right_id")),
            ),
        ]


Review comment (suggestion):
EDIT ~The same flag should be checked in `test_check_constraint_refs_excluded_field_attname`.~ Nvd

Code after:
        required_db_features = {"supports_table_check_constraints"}
        constraints = [
            models.UniqueConstraint(
                "name",
                "category",
                name="unique_name_category",
                violation_error_message="This product already exists.",
            ),
            models.CheckConstraint(
                condition=models.Q(price__gt=0),
                name="price_gte_zero",
                violation_error_message="Price must be greater than zero.",
            ),
        ]


class AttnameConstraintsModel(models.Model):
    left = models.ForeignKey(
        "self", related_name="+", null=True, on_delete=models.SET_NULL
    )
    right = models.ForeignKey(
        "self", related_name="+", null=True, on_delete=models.SET_NULL
    )

    class Meta:
        required_db_features = {"supports_table_check_constraints"}
        constraints = [
            models.CheckConstraint(
                name="%(app_label)s_%(class)s_left_not_right",
                # right_id here is the ForeignKey's attname, not name.
                condition=~models.Q(left=models.F("right_id")),
            ),
        ]
Repository: django/django
File: django/utils/feedgenerator.py (Python)

Code before:
        self._mimetype = mimetype
        self.media = media

    # Using a property to delay the evaluation of self._url as late as possible
    # in case of a lazy object (like reverse_lazy(...) for example).
    @property
    def url(self):
        return iri_to_uri(self._url)

    @property
    def mimetype(self):
        if self._mimetype == "":
            return _guess_stylesheet_mimetype(self.url)[0]
        return self._mimetype

    def __str__(self):
        url = escape(iri_to_uri(self._url))
        mimetype = escape(self.mimetype) if self.mimetype is not None else None
        media = escape(self.media) if self.media is not None else None

        data = [f'href="{url}"']
        if mimetype is not None:
            data.append(f'type="{mimetype}"')
        if media is not None:
            data.append(f'media="{media}"')
        return " ".join(data)

    def __repr__(self):
        return repr((self.url, self.mimetype, self.media))


class SyndicationFeed:
    "Base class for all syndication feeds. Subclasses should provide write()"

    def __init__(
        self,
        title,
        link,
        description,
        language=None,
        author_email=None,
        author_name=None,
        author_link=None,
        subtitle=None,
        categories=None,
        feed_url=None,
        feed_copyright=None,
        feed_guid=None,
        ttl=None,
        stylesheets=None,
        **kwargs,

Review comment (suggestion):
Suggestion: It might be nice to use `django.forms.utils.flatatt` here, since it handles both joining the values together and escaping them

Code after:
        self._mimetype = mimetype
        self.media = media

    # Using a property to delay the evaluation of self._url as late as possible
    # in case of a lazy object (like reverse_lazy(...) for example).
    @property
    def url(self):
        return iri_to_uri(self._url)

    @property
    def mimetype(self):
        if self._mimetype == "":
            return _guess_stylesheet_mimetype(self.url)[0]
        return self._mimetype

    def __str__(self):
        attrs = {
            "href": iri_to_uri(self._url),
            "type": self.mimetype,
            "media": self.media,
        }
        return flatatt(attrs).strip()

    def __repr__(self):
        return repr((self.url, self.mimetype, self.media))


class SyndicationFeed:
    "Base class for all syndication feeds. Subclasses should provide write()"

    def __init__(
        self,
        title,
        link,
        description,
        language=None,
        author_email=None,
        author_name=None,
        author_link=None,
        subtitle=None,
        categories=None,
        feed_url=None,
        feed_copyright=None,
        feed_guid=None,
        ttl=None,
        stylesheets=None,
        **kwargs,
    ):
        def to_str(s):
            return str(s) if s is not None else s
Repository: django/django
File: django/db/backends/sqlite3/_functions.py (Python)

Code before:
"""
Implementations of SQL functions for SQLite.
"""

import functools
import random
import statistics
import uuid
import zoneinfo
from datetime import timedelta
from hashlib import md5, sha1, sha224, sha256, sha384, sha512
from math import (
    acos,
    asin,
    atan,
    atan2,
    ceil,
    cos,
    degrees,
    exp,
    floor,
    fmod,
    log,
    pi,
    radians,
    sin,
    sqrt,
    tan,
)
from re import search as re_search

from django.db.backends.utils import (
    split_tzname_delta,

Review comment (suggestion):
Do `from uuid import uuid4, uuid7` to avoid the attribute lookup in the functions - micro optimisation but since these functions can be called many times in a query I think it makes sense. uuid7 will need guarding with PY314.

Code after:
"""
Implementations of SQL functions for SQLite.
"""

import functools
import random
import statistics
import zoneinfo
from datetime import timedelta
from hashlib import md5, sha1, sha224, sha256, sha384, sha512
from math import (
    acos,
    asin,
    atan,
    atan2,
    ceil,
    cos,
    degrees,
    exp,
    floor,
    fmod,
    log,
    pi,
    radians,
    sin,
    sqrt,
    tan,
)
from re import search as re_search
from uuid import uuid4

from django.db.backends.utils import (
    split_tzname_delta,
Repository: django/django
File: django/db/backends/oracle/features.py (Python)

Code before:
        return self.connection.oracle_version >= (21,)

    @cached_property
    def supports_boolean_expr_in_select_clause(self):
        return self.connection.oracle_version >= (23,)

    @cached_property
    def supports_comparing_boolean_expr(self):
        return self.connection.oracle_version >= (23,)

    @cached_property
    def supports_aggregation_over_interval_types(self):
        return self.connection.oracle_version >= (23,)

    @cached_property
    def bare_select_suffix(self):
        return "" if self.connection.oracle_version >= (23,) else " FROM DUAL"

    @cached_property
    def supports_tuple_lookups(self):
        # Support is known to be missing on 23.2 but available on 23.4.
        return self.connection.oracle_version >= (23, 4)

    @cached_property
    def supports_uuid4_function(self):
        return self.connection.oracle_version >= (23, 4)


Review comment (suggestion):
```suggestion
        return self.connection.oracle_version >= (23, 9)
```

Code after:
        return self.connection.oracle_version >= (21,)

    @cached_property
    def supports_boolean_expr_in_select_clause(self):
        return self.connection.oracle_version >= (23,)

    @cached_property
    def supports_comparing_boolean_expr(self):
        return self.connection.oracle_version >= (23,)

    @cached_property
    def supports_aggregation_over_interval_types(self):
        return self.connection.oracle_version >= (23,)

    @cached_property
    def bare_select_suffix(self):
        return "" if self.connection.oracle_version >= (23,) else " FROM DUAL"

    @cached_property
    def supports_tuple_lookups(self):
        # Support is known to be missing on 23.2 but available on 23.4.
        return self.connection.oracle_version >= (23, 4)

    @cached_property
    def supports_uuid4_function(self):
        return self.connection.oracle_version >= (23, 9)
Repository: django/django
File: tests/postgres_tests/test_functions.py (Python)

Code before:
from . import PostgreSQLTestCase
from .models import NowTestModel


class TestTransactionNow(PostgreSQLTestCase):
    def test_transaction_now(self):
        """
        The test case puts everything under a transaction, so two models
        updated with a short gap should have the same time.
        """
        m1 = NowTestModel.objects.create()
        m2 = NowTestModel.objects.create()

        NowTestModel.objects.filter(id=m1.id).update(when=TransactionNow())
        sleep(0.1)
        NowTestModel.objects.filter(id=m2.id).update(when=TransactionNow())

        m1.refresh_from_db()
        m2.refresh_from_db()

        self.assertIsInstance(m1.when, datetime)
        self.assertEqual(m1.when, m2.when)


class TestRandomUUID(PostgreSQLTestCase):
    def test_deprecated(self):
        msg = "Use django.db.models.functions.UUID4 instead."
        with self.assertWarnsMessage(RemovedInDjango70Warning, msg):
            RandomUUID()


Review comment (suggestion):
Let's keep the new test for the deprecation warning but also keep the old test since it's still supported for now. All you should have to do is `warnings.catch_warnings(action="ignore", category=...)`

Code after:
from . import PostgreSQLTestCase
from .models import NowTestModel, UUIDTestModel


class TestTransactionNow(PostgreSQLTestCase):
    def test_transaction_now(self):
        """
        The test case puts everything under a transaction, so two models
        updated with a short gap should have the same time.
        """
        m1 = NowTestModel.objects.create()
        m2 = NowTestModel.objects.create()

        NowTestModel.objects.filter(id=m1.id).update(when=TransactionNow())
        sleep(0.1)
        NowTestModel.objects.filter(id=m2.id).update(when=TransactionNow())

        m1.refresh_from_db()
        m2.refresh_from_db()

        self.assertIsInstance(m1.when, datetime)
        self.assertEqual(m1.when, m2.when)


class TestRandomUUID(PostgreSQLTestCase):
    def test_random_uuid(self):
        m1 = UUIDTestModel.objects.create()
        m2 = UUIDTestModel.objects.create()
        UUIDTestModel.objects.update(uuid=RandomUUID())
        m1.refresh_from_db()
        m2.refresh_from_db()
        self.assertIsInstance(m1.uuid, uuid.UUID)
        self.assertNotEqual(m1.uuid, m2.uuid)
Repository: django/django
File: django/db/models/functions/uuid.py (Python)

Code before:
from django.db import NotSupportedError
from django.db.models.expressions import Func
from django.db.models.fields import UUIDField


class UUID4(Func):
    function = "UUIDV4"
    output_field = UUIDField()

    def as_sql(self, compiler, connection, **extra_context):
        if connection.features.supports_uuid4_function:
            return super().as_sql(compiler, connection, **extra_context)
        raise NotSupportedError("UUID4 is not supported on this database backend.")

    def as_postgresql(self, compiler, connection, **extra_context):
        if connection.features.is_postgresql_18:
            return self.as_sql(compiler, connection, **extra_context)
        return self.as_sql(
            compiler, connection, function="GEN_RANDOM_UUID", **extra_context
        )

    def as_mysql(self, compiler, connection, **extra_context):
        if not connection.features.supports_uuid4_function:
            if connection.mysql_is_mariadb:
                raise NotSupportedError("UUID4 requires MariaDB version 11.7 or later.")
            raise NotSupportedError("UUID4 is not supported on MySQL.")
        return self.as_sql(compiler, connection, function="UUID_V4", **extra_context)

    def as_oracle(self, compiler, connection, **extra_context):
        if not connection.features.supports_uuid4_function:
            raise NotSupportedError("UUID4 requires Oracle version 23ai or later.")
        return self.as_sql(compiler, connection, function="UUID", **extra_context)

Review comment (suggestion):
We should take the opportunity to add `arity` on new built-in functions:
```suggestion
    function = "UUIDV4"
    arity = 0
```

Code after:
from django.db import NotSupportedError
from django.db.models.expressions import Func
from django.db.models.fields import UUIDField


class UUID4(Func):
    function = "UUIDV4"
    arity = 0
    output_field = UUIDField()

    def as_sql(self, compiler, connection, **extra_context):
        if connection.features.supports_uuid4_function:
            return super().as_sql(compiler, connection, **extra_context)
        raise NotSupportedError("UUID4 is not supported on this database backend.")

    def as_postgresql(self, compiler, connection, **extra_context):
        if connection.features.is_postgresql_18:
            return self.as_sql(compiler, connection, **extra_context)
        return self.as_sql(
            compiler, connection, function="GEN_RANDOM_UUID", **extra_context
        )

    def as_mysql(self, compiler, connection, **extra_context):
        if not connection.features.supports_uuid4_function:
            if connection.mysql_is_mariadb:
                raise NotSupportedError("UUID4 requires MariaDB version 11.7 or later.")
            raise NotSupportedError("UUID4 is not supported on MySQL.")
        return self.as_sql(compiler, connection, function="UUID_V4", **extra_context)

    def as_oracle(self, compiler, connection, **extra_context):
        if not connection.features.supports_uuid4_function:
            raise NotSupportedError(
Repository: django/django
File: django/contrib/postgres/functions.py (Python)

Code before:
import warnings

from django.db.models import DateTimeField, Func
from django.db.models.functions import UUID4
from django.utils.deprecation import RemovedInDjango70Warning, django_file_prefixes


class RandomUUID(UUID4):
    def __init__(self, *args, **kwargs):
        warnings.warn(
            "RandomUUID is deprecated. Use django.db.models.functions.UUID4 instead.",
            RemovedInDjango70Warning,
            skip_file_prefixes=django_file_prefixes(),
        )
        super().__init__(*args, **kwargs)


class TransactionNow(Func):
    template = "CURRENT_TIMESTAMP"
    output_field = DateTimeField()


Review comment (suggestion):
I don't think it's worth removing these class attributes -- we're breaking backward compatibility for subclasses that might be using different `template` or `function` or `output_field`.

Code after:
from django.db.models import DateTimeField, Func, UUIDField


class RandomUUID(Func):
    template = "GEN_RANDOM_UUID()"
    output_field = UUIDField()


class TransactionNow(Func):
    template = "CURRENT_TIMESTAMP"
    output_field = DateTimeField()