ronantakizawa/codereview-bench huggingface dataset:
Repository: pypa/pip
File: build-project.py (Python)

Code before:


def get_git_head_timestamp() -> str:
    return subprocess.run(
        [
            "git",
            "log",
            "-1",
            "--pretty=format:%ct",
        ],
        text=True,
        stdout=subprocess.PIPE,
    ).stdout.strip()


def main() -> None:
    with tempfile.TemporaryDirectory() as build_env:
        subprocess.run(
            [
                sys.executable,
                "-m",
                "venv",
                build_env,
            ],
            check=True,
        )
        build_python = Path(build_env) / "bin" / "python"
        subprocess.run(
            [
                build_python,
                "-Im",
                "pip",
                "install",
                "--no-deps",
                "--only-binary=:all:",
                "--require-hashes",
                "-r",
                Path(__file__).parent / "build-requirements.txt",
            ],
            check=True,
        )
        subprocess.run(
            [
                build_python,
                "-Im",
                "build",
                "--no-isolation",
            ],
            check=True,
            env={"SOURCE_DATE_EPOCH": get_git_head_timestamp()},
        )

Review comment (suggestion):
I know that I literally used to maintain Black, but I never liked how Black splits long lists into sprawling messes like this. I would be in favour of using `# fmt: off/on` (or `# fmt: skip`) here. 

Code after:
from pathlib import Path
from types import SimpleNamespace


class EnvBuilder(venv.EnvBuilder):
    """A subclass of venv.EnvBuilder that exposes the python executable command."""

    def ensure_directories(
        self, env_dir: str | bytes | PathLike[str] | PathLike[bytes]
    ) -> SimpleNamespace:
        context = super().ensure_directories(env_dir)
        self.env_exec_cmd = context.env_exec_cmd
        return context


def get_git_head_timestamp() -> str:
    return subprocess.run(
        [
            "git",
            "log",
            "-1",
            "--pretty=format:%ct",
        ],
        text=True,
        stdout=subprocess.PIPE,
    ).stdout.strip()


def main() -> None:
    with tempfile.TemporaryDirectory() as build_env:
        env_builder = EnvBuilder(with_pip=True)
        env_builder.create(build_env)
        subprocess.run(
            [
                env_builder.env_exec_cmd,
                "-Im",
                "pip",
                "install",
                "--no-deps",
                "--only-binary=:all:",
                "--require-hashes",
                "-r",
                Path(__file__).parent / "build-requirements.txt",
            ],
            check=True,
        )
        subprocess.run(
            [
                env_builder.env_exec_cmd,
                "-Im",
                "build",
Repository: pypa/pip
File: tests/functional/test_install_script.py (Python)

Code before:
            # requires-python = ">={target_python_ver}"
            # dependencies = [
            #   "INITools==0.2",
            #   "{other_lib_name}<={other_lib_version}",
            # ]
            # ///

            print("Hello world from a dummy program")
            """
        )
    )

    result = script.pip(
        "install",
        "--requirements-from-script",
        script_path,
        expect_stderr=True,
        expect_error=True,
    )

    if sys.platform == "win32":
        # Special case: result.stderr contains an extra layer of backslash
        # escaping, transform our path to match
        script_path_str = str(script_path).replace("\\", "\\\\")
    else:
        script_path_str = str(script_path)

    assert (
        f"ERROR: Script '{script_path_str}' requires a different Python"
        in result.stderr
    ), (
        "Script with incompatible requires-python did not fail as expected -- "
        + result.stderr
    )


Review comment (suggestion):
Seems like there may be a more elegant way to deal with the extra layer of escaping, but I couldn't spot anything better in the test suite so I went with this.

Code after:
    result = script.pip_install_local(
        "--requirements-from-script",
        script_path,
        expect_stderr=True,
        expect_error=True,
    )

    assert "requires a different Python" in result.stderr, (
        "Script with incompatible requires-python did not fail as expected -- "
        + result.stderr
    )


def test_script_invalid_TOML(script: PipTestEnvironment) -> None:
    """
    Test installing from a script with invalid TOML in its 'script' metadata
    """

    script_path = script.scratch_path.joinpath("script.py")

    script_path.write_text(
        textwrap.dedent(
            f"""\
            # /// script
            # requires-python = "!={sys.version_info.major}.{sys.version_info.minor}.*"
            # dependencies = [
            # ///

            print("Hello world from a dummy program")
            """
        )
    )

    result = script.pip_install_local(
        "--requirements-from-script",
        script_path,
        expect_stderr=True,
        expect_error=True,
    )

    assert "Failed to parse TOML" in result.stderr, (
        "Script with invalid TOML metadata did not fail as expected -- " + result.stderr
    )


def test_script_multiple_blocks(script: PipTestEnvironment) -> None:
    """
    Test installing from a script with multiple metadata blocks
    """

    script_path = script.scratch_path.joinpath("script.py")
Repository: pypa/pip
File: tests/functional/test_completion.py (Python)

Code before:
    assert executable_name in result.stdout


@pytest.mark.parametrize(
    "subcommand, handler_prefix, expected",
    [
        ("cache", "d", "dir"),
        ("cache", "in", "info"),
        ("cache", "l", "list"),
        ("cache", "re", "remove"),
        ("cache", "pu", "purge"),
        ("config", "li", "list"),
        ("config", "e", "edit"),
        ("config", "ge", "get"),
        ("config", "se", "set"),
        ("config", "unse", "unset"),
        ("config", "d", "debug"),
        ("index", "ve", "versions"),
    ],
)
def test_completion_for_action_handler(
    subcommand: str, handler_prefix: str, expected: str, autocomplete: DoAutocomplete
) -> None:
    res, _ = autocomplete(f"pip {subcommand} {handler_prefix}", cword="2")

    assert [expected] == res.stdout.split()


Review comment (suggestion):
this list could probably be auto-generated from all commands with a non-empty `handler_map` but I just hard coded it for simplicity (read: laziness :upside_down_face:)

Code after:
    executable_name = f"pip{sys.version_info[0]}"
    # Deprecated python versions produce an extra deprecation warning
    result = autocomplete_script.run(
        executable_name,
        "completion",
        flag,
        expect_stderr=deprecated_python,
    )
    assert executable_name in result.stdout


@pytest.mark.parametrize(
    "subcommand, handler_prefix, expected",
    [
        ("cache", "d", "dir"),
        ("cache", "in", "info"),
        ("cache", "l", "list"),
        ("cache", "re", "remove"),
        ("cache", "pu", "purge"),
        ("config", "li", "list"),
        ("config", "e", "edit"),
        ("config", "ge", "get"),
        ("config", "se", "set"),
        ("config", "unse", "unset"),
        ("config", "d", "debug"),
        ("index", "ve", "versions"),
    ],
)
def test_completion_for_action_handler(
    subcommand: str, handler_prefix: str, expected: str, autocomplete: DoAutocomplete
) -> None:
    res, _ = autocomplete(f"pip {subcommand} {handler_prefix}", cword="2")

    assert [expected] == res.stdout.split()


def test_completion_for_action_handler_handler_not_repeated(
    autocomplete: DoAutocomplete,
) -> None:
    res, _ = autocomplete("pip cache remove re", cword="3")

    assert [] == res.stdout.split()
Repository: pypa/pip
File: docs/html/conf.py (Python)

Code before:
"""Sphinx configuration file for pip's documentation."""

import glob
import os
import pathlib
import re
import sys
from typing import List, Tuple

# Add the docs/ directory to sys.path, because pip_sphinxext.py is there.
docs_dir = os.path.dirname(os.path.dirname(__file__))
sys.path.insert(0, docs_dir)

# -- General configuration ------------------------------------------------------------
extensions = [
    # extensions common to 'man' and 'html' builds
    "pip_sphinxext",
]

# 'tags' is a special object handled by sphinx
# https://www.sphinx-doc.org/en/master/usage/configuration.html#project-tags
if "html" in tags or "READTHEDOCS" in os.environ:  # type: ignore[name-defined] # noqa: F821
    # html specific deps
    extensions.extend(
        (
            # first-party extensions
            "sphinx.ext.autodoc",
            "sphinx.ext.todo",
            "sphinx.ext.extlinks",
            "sphinx.ext.intersphinx",
            # third-party extensions
            "myst_parser",
            "sphinx_copybutton",
            "sphinx_inline_tabs",
            "sphinxcontrib.towncrier",
        ),
    )

# General information about the project.
project = "pip"
copyright = "The pip developers"

Review comment (suggestion):
I'd also recommend keeping the order of extensions: first-party -> third-party -> local. Just like with regular imports.

Code after:
"""Sphinx configuration file for pip's documentation."""

import glob
import os
import pathlib
import re
import sys
from typing import List, Tuple

# Add the docs/ directory to sys.path, because pip_sphinxext.py is there.
docs_dir = os.path.dirname(os.path.dirname(__file__))
sys.path.insert(0, docs_dir)

# -- General configuration ------------------------------------------------------------

extensions = [
    # extensions common to all builds
    "pip_sphinxext",
]

# 'tags' is a injected by sphinx
# https://www.sphinx-doc.org/en/master/usage/configuration.html#project-tags
if "man" not in tags:  # type: ignore[name-defined] # noqa: F821
    # extensions not needed for building man pages
    extensions.extend(
        (
            # first-party extensions
            "sphinx.ext.autodoc",
            "sphinx.ext.todo",
            "sphinx.ext.intersphinx",
            # third-party extensions
            "myst_parser",
            "sphinx_copybutton",
            "sphinx_inline_tabs",
            "sphinxcontrib.towncrier",
            "sphinx_issues",
        ),
    )

# General information about the project.
project = "pip"
Repository: pypa/pip
File: src/pip/_internal/commands/index.py (Python)

Code before:
            versions = set(versions)

            if not versions:
                raise DistributionNotFound(
                    f"No matching distribution found for {query}"
                )

            formatted_versions = [str(ver) for ver in sorted(versions, reverse=True)]
            latest = formatted_versions[0]

        if options.json:
            env = get_default_environment()
            dist = env.get_distribution(query)
            structured_output = {
                "name": query,
                "versions": formatted_versions,
                "latest": latest,
            }

            if dist is not None:
                structured_output["installed_version"] = str(dist.version)

            write_output(json.dumps(structured_output))
            return

        write_output(f"{query} ({latest})")
        write_output("Available versions: {}".format(", ".join(formatted_versions)))
        print_dist_installation_info(query, latest)


Review comment (refactor):
Can this be structured as follows:

```
if options.json:
    ...
else:
    ...
```

rather than

```
if options.json:
    ...
    return
...
```

as the former is (IMO) easier to read.

I'm also slightly uncomfortable with the fact that we're duplicating the lookup of `dist` here and in `print_dist_installation_info`. Ideally, I'd like to see this refactored to remove the duplication. But it's not a showstopper, and if you prefer it could be deferred to a follow-up PR.

Code after:
                    version for version in versions if not version.is_prerelease
                )
            versions = set(versions)

            if not versions:
                raise DistributionNotFound(
                    f"No matching distribution found for {query}"
                )

            formatted_versions = [str(ver) for ver in sorted(versions, reverse=True)]
            latest = formatted_versions[0]

        dist = get_installed_distribution(query)

        if options.json:
            structured_output = {
                "name": query,
                "versions": formatted_versions,
                "latest": latest,
            }

            if dist is not None:
                structured_output["installed_version"] = str(dist.version)

            write_output(json.dumps(structured_output))

        else:
            write_output(f"{query} ({latest})")
            write_output("Available versions: {}".format(", ".join(formatted_versions)))
            print_dist_installation_info(latest, dist)
Repository: pypa/pip
File: tools/update-rtd-redirects.py (Python)

Code before:
"""Update the 'exact' redirects on Read the Docs to match an in-tree file's contents.

Relevant API reference: https://docs.readthedocs.io/en/stable/api/v3.html#redirects
"""

# /// script
# requires-python = ">=3.8"
# dependencies = ["httpx", "rich", "pyyaml"]
# ///

import operator
import os
import sys
from pathlib import Path
from typing import Dict, List

import httpx
import rich
import yaml

try:
    _TOKEN = os.environ["RTD_API_TOKEN"]
except KeyError:
    rich.print(
        "[bold]error[/]: [red]No API token provided. Please set `RTD_API_TOKEN`.[/]",
        file=sys.stderr,
    )
    sys.exit(1)

RTD_API_HEADERS = {"Authorization": f"token {_TOKEN}"}
RTD_API_BASE_URL = "https://readthedocs.org/api/v3/projects/pip/"
REPO_ROOT = Path(__file__).resolve().parent.parent

Review comment (suggestion):
```suggestion
# requires-python = ">=3.11"
```

Given that this is just an "internal-to-us" tool, let's use what CI allows us to use.

Code after:
"""Update the 'exact' redirects on Read the Docs to match an in-tree file's contents.

Relevant API reference: https://docs.readthedocs.io/en/stable/api/v3.html#redirects
"""

# /// script
# requires-python = ">=3.11"
# dependencies = ["httpx", "rich", "pyyaml"]
# ///

import operator
import os
import sys
from pathlib import Path
from typing import Dict, List

import httpx
import rich
import yaml

try:
    _TOKEN = os.environ["RTD_API_TOKEN"]
except KeyError:
    rich.print(
        "[bold]error[/]: [red]No API token provided. Please set `RTD_API_TOKEN`.[/]",
        file=sys.stderr,
    )
    sys.exit(1)

RTD_API_HEADERS = {"Authorization": f"token {_TOKEN}"}
RTD_API_BASE_URL = "https://readthedocs.org/api/v3/projects/pip/"
REPO_ROOT = Path(__file__).resolve().parent.parent
Repository: pypa/pip
File: src/pip/_internal/utils/filesystem.py (Python)

Code before:
    return os.path.getsize(path)


def format_file_size(path: str) -> str:
    return format_size(file_size(path))


def directory_size(path: str) -> int | float:
    size = 0.0
    for root, _dirs, files in os.walk(path):
        for filename in files:
            file_path = os.path.join(root, filename)
            size += file_size(file_path)
    return size


def format_directory_size(path: str) -> str:
    return format_size(directory_size(path))


def copy_directory_permissions(directory: str, target_file: BinaryIO) -> None:
    mode = (
        os.stat(directory).st_mode
        & 0o666  # select read/write permissions of cache directory
        | 0o600  # set owner read/write permissions
    )
    # Change permissions only if there is no risk of following a symlink.
    if os.chmod in os.supports_fd:
        os.chmod(target_file.fileno(), mode)
    elif os.chmod in os.supports_follow_symlinks:
        os.chmod(target_file.name, mode, follow_symlinks=False)


Review comment (security):
```suggestion
        os.stat(directory).st_mode
        & 0o666  # select read/write permissions of directory
        | 0o600  # set owner read/write permissions
    )
```

Code after:
    return os.path.getsize(path)


def format_file_size(path: str) -> str:
    return format_size(file_size(path))


def directory_size(path: str) -> int | float:
    size = 0.0
    for root, _dirs, files in os.walk(path):
        for filename in files:
            file_path = os.path.join(root, filename)
            size += file_size(file_path)
    return size


def format_directory_size(path: str) -> str:
    return format_size(directory_size(path))


def copy_directory_permissions(directory: str, target_file: BinaryIO) -> None:
    mode = (
        os.stat(directory).st_mode & 0o666  # select read/write permissions of directory
        | 0o600  # set owner read/write permissions
    )
    # Change permissions only if there is no risk of following a symlink.
    if os.chmod in os.supports_fd:
        os.chmod(target_file.fileno(), mode)
    elif os.chmod in os.supports_follow_symlinks:
        os.chmod(target_file.name, mode, follow_symlinks=False)
Repository: pypa/pip
File: src/pip/_internal/exceptions.py (Python)

Code before:
    def __init__(self, failed: list[InstallRequirement]) -> None:
        super().__init__(
            message=(
                "Failed to build installable wheels for some "
                "pyproject.toml based projects"
            ),
            context=", ".join(r.name for r in failed),  # type: ignore
            hint_stmt=None,
        )


class InvalidEggFragment(DiagnosticPipError):
    reference = "invalid-egg-fragment"

    def __init__(self, link: Link, fragment: str) -> None:
        if ">" in fragment or "=" in fragment or "<" in fragment:
            hint = (
                "Version specifiers are silently ignored for URL references. "
                "Remove them."
            )
        elif fragment.endswith("]"):
            stripped = re.sub(r"[#&]egg=([^&]*)", "", str(link))
            hint = (
                "Try using the Direct URL requirement syntax: "
                f"'{fragment} @ {stripped}'"
            )
        else:
            hint = "Egg fragments can only be a valid project name."

        super().__init__(
            message=f"The '{escape(fragment)}' egg fragment is invalid",
            context=f"from '{escape(str(link))}'",
            hint_stmt=escape(hint),
        )


Review comment (suggestion):
The reason why I'm using `re.sub` instead of a split is to preserve `#subdirectory=dir` fragments. 

```
pip install -e "pip @ file:///home/ichard26/dev/oss#subdirectory=pip"
```

Code after:

    def __init__(self, failed: list[InstallRequirement]) -> None:
        super().__init__(
            message=(
                "Failed to build installable wheels for some "
                "pyproject.toml based projects"
            ),
            context=", ".join(r.name for r in failed),  # type: ignore
            hint_stmt=None,
        )


class InvalidEggFragment(DiagnosticPipError):
    reference = "invalid-egg-fragment"

    def __init__(self, link: Link, fragment: str) -> None:
        hint = ""
        if ">" in fragment or "=" in fragment or "<" in fragment:
            hint = (
                "Version specifiers are silently ignored for URL references. "
                "Remove them. "
            )
        if "[" in fragment and "]" in fragment:
            hint += "Try using the Direct URL requirement syntax: 'name[extra] @ URL'"

        if not hint:
            hint = "Egg fragments can only be a valid project name."

        super().__init__(
            message=f"The '{escape(fragment)}' egg fragment is invalid",
            context=f"from '{escape(str(link))}'",
            hint_stmt=escape(hint),
        )


class BuildDependencyInstallError(DiagnosticPipError):
    """Raised when build dependencies cannot be installed."""

    reference = "failed-build-dependency-install"

    def __init__(
        self,
        req: InstallRequirement | None,
        build_reqs: Iterable[str],
        *,
        cause: Exception,
        log_lines: list[str] | None,
    ) -> None:
        if isinstance(cause, PipError):
            note = "This is likely not a problem with pip."
        else:
Repository: pyscript/pyscript
File: pyscriptjs/src/utils.ts (TypeScript)

Code before:
function addClasses(element: HTMLElement, classes: Array<string>) {
    for (const entry of classes) {
        element.classList.add(entry);
    }
}

function removeClasses(element: HTMLElement, classes: Array<string>) {
    for (const entry of classes) {
        element.classList.remove(entry);
    }
}

function getLastPath(str: string): string {
    return str.split('\\').pop().split('/').pop();
}

function htmlDecode(input: string): string {
    const doc = new DOMParser().parseFromString(ltrim(input), 'text/html');
    return doc.documentElement.textContent;
}

function ltrim(code: string): string {
    const lines = code.split('\n');
    if (lines.length == 0) return code;

    const lengths = lines
        .filter(line => line.trim().length != 0)
        .map(line => {
            const [prefix] = line.match(/^\s*/);
            return prefix.length;
        });

    const k = Math.min(...lengths);

    return k != 0 ? lines.map(line => line.substring(k)).join('\n')
                  : code;
}

function guidGenerator(): string {
    const S4 = function (): string {
        return (((1 + Math.random()) * 0x10000) | 0).toString(16).substring(1);
    };
    return S4() + S4() + '-' + S4() + '-' + S4() + '-' + S4() + '-' + S4() + S4() + S4();
}


Review comment (bug):
I think we should escape more?
I'm not an expert in the field, but a quick googling found this:
https://stackoverflow.com/a/6234804

I guess we should probably escape `'`, `"` and `&` as well?


Code after:
function addClasses(element: HTMLElement, classes: Array<string>) {
    for (const entry of classes) {
        element.classList.add(entry);
    }
}

function removeClasses(element: HTMLElement, classes: Array<string>) {
    for (const entry of classes) {
        element.classList.remove(entry);
    }
}

function getLastPath(str: string): string {
    return str.split('\\').pop().split('/').pop();
}

function escape(str: string): string {
    return str.replace(/</g, "&lt;").replace(/>/g, "&gt;")
}

function htmlDecode(input: string): string {
    const doc = new DOMParser().parseFromString(ltrim(escape(input)), 'text/html');
    return doc.documentElement.textContent;
}

function ltrim(code: string): string {
    const lines = code.split('\n');
    if (lines.length == 0) return code;

    const lengths = lines
        .filter(line => line.trim().length != 0)
        .map(line => {
            const [prefix] = line.match(/^\s*/);
            return prefix.length;
        });

    const k = Math.min(...lengths);

    return k != 0 ? lines.map(line => line.substring(k)).join('\n')
                  : code;
}

function guidGenerator(): string {
    const S4 = function (): string {
        return (((1 + Math.random()) * 0x10000) | 0).toString(16).substring(1);