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

Code before:
# from __future__ import annotations

import asyncio
import datetime
import hashlib
import inspect

# Try to import the xxhash library as an optional dependency
try:
    import xxhash

    HAS_XXHASH = True
except ImportError:
    HAS_XXHASH = False

import warnings
from enum import Enum
from typing import (
    TYPE_CHECKING,
    Any,
    AsyncIterator,
    Awaitable,
    Callable,
    Dict,
    Iterable,
    Iterator,
    List,
    Literal,

Review comment (suggestion):
The asyncio import is unused and should be removed. The inspect module is used for checking coroutines, but asyncio is not directly used in this file.
```suggestion

```

Code after:
# from __future__ import annotations

import datetime
import hashlib
import inspect

# Try to import the xxhash library as an optional dependency
try:
    import xxhash

    HAS_XXHASH = True
except ImportError:
    HAS_XXHASH = False

import warnings
from enum import Enum
from typing import (
    TYPE_CHECKING,
    Any,
    AsyncIterator,
    Awaitable,
    Callable,
    Dict,
    Iterable,
    Iterator,
    List,
    Literal,
    Mapping,
Repository: remix-run/react-router
File: packages/react-router/__tests__/useLocation-test.tsx (TypeScript)

Code before:
function ShowLocation() {
  let location = useLocation();
  return <pre>{JSON.stringify(location)}</pre>;
}

describe("useLocation", () => {
  it("returns the current location object", () => {
    let renderer: TestRenderer.ReactTestRenderer;
    TestRenderer.act(() => {
      renderer = TestRenderer.create(
        <MemoryRouter initialEntries={["/home?the=search#the-hash"]}>
          <Routes>
            <Route path="/home" element={<ShowLocation />} />
          </Routes>
        </MemoryRouter>
      );
    });

    expect(renderer.toJSON()).toMatchInlineSnapshot(`
      <pre>
        {"pathname":"/home","search":"?the=search","hash":"#the-hash","state":null,"key":"default"}
      </pre>
    `);
  });

  it("returns the scoped location object when nested in <Routes location>", () => {
    let renderer: TestRenderer.ReactTestRenderer;
    TestRenderer.act(() => {
      renderer = TestRenderer.create(
        <MemoryRouter initialEntries={["/home?the=search#the-hash"]}>
          <App />
        </MemoryRouter>
      );
    });

    function App() {
      return (
        <div>
          <h1>App</h1>
          <Routes>
            <Route path="/home" element={<ShowLocation />} />
          </Routes>
          <Routes location="/scoped?scoped=search#scoped-hash">
            <Route path="/scoped" element={<ShowLocation />} />
          </Routes>
        </div>
      );
    }

    expect(renderer.toJSON()).toMatchInlineSnapshot(`
      <div>

Review comment (suggestion):
Removed the other test modification in favor of a dedicated `useLocation` test here 

Code after:
function ShowLocation() {
  let location = useLocation();
  return <pre>{JSON.stringify(location)}</pre>;
}

describe("useLocation", () => {
  it("returns the current location object", () => {
    let renderer: TestRenderer.ReactTestRenderer;
    TestRenderer.act(() => {
      renderer = TestRenderer.create(
        <MemoryRouter initialEntries={["/home?the=search#the-hash"]}>
          <Routes>
            <Route path="/home" element={<ShowLocation />} />
          </Routes>
        </MemoryRouter>
      );
    });

    expect(renderer.toJSON()).toMatchInlineSnapshot(`
      <pre>
        {"pathname":"/home","search":"?the=search","hash":"#the-hash","state":null,"key":"default"}
      </pre>
    `);
  });

  it("returns the scoped location object when nested in <Routes location>", () => {
    let renderer: TestRenderer.ReactTestRenderer;
    TestRenderer.act(() => {
      renderer = TestRenderer.create(
        <MemoryRouter initialEntries={["/home?the=search#the-hash"]}>
          <App />
        </MemoryRouter>
      );
    });

    function App() {
      return (
        <div>
          <Routes>
            <Route path="/home" element={<ShowLocation />} />
          </Routes>
          <Routes location="/scoped?scoped=search#scoped-hash">
            <Route path="/scoped" element={<ShowLocation />} />
          </Routes>
        </div>
      );
    }

    expect(renderer.toJSON()).toMatchInlineSnapshot(`
      <div>
        <pre>
Repository: remix-run/react-router
File: packages/router/utils.ts (TypeScript)

Code before:
        // the entire path
        return str === "/*" ? "/" : "";
      }

      // Apply the splat
      return `${prefix}${params[star]}`;
    });
}

/**
 * A PathPattern is used to match on some portion of a URL pathname.
 */
export interface PathPattern<Path extends string = string> {
  /**
   * A string to match against a URL pathname. May contain `:id`-style segments
   * to indicate placeholders for dynamic parameters. May also end with `/*` to
   * indicate matching the rest of the URL pathname.
   */
  path: Path;
  /**
   * Should be `true` if the static portions of the `path` should be matched in
   * the same case.
   */
  caseSensitive?: boolean;
  /**
   * Should be `true` if this pattern should match the entire URL pathname.
   */
  end?: boolean;
}

/**
 * A PathMatch contains info about how a PathPattern matched on a URL pathname.
 */
export interface PathMatch<ParamKey extends string = string> {
  /**
   * The names and values of dynamic parameters in the URL.
   */
  params: Params<ParamKey>;
  /**
   * The portion of the URL pathname that was matched.
   */
  pathname: string;
  /**
   * The portion of the URL pathname that was matched before child routes.
   */
  pathnameBase: string;
  /**
   * The pattern that was used to match.
   */
  pattern: PathPattern;
}

Review comment (suggestion):
Same logic as we use in `compilePath` for partial splats

Code after:
  }

  return matches;
}

/**
 * Returns a path with params interpolated.
 *
 * @see https://reactrouter.com/docs/en/v6/utils/generate-path
 */
export function generatePath<Path extends string>(
  originalPath: Path,
  params: {
    [key in PathParam<Path>]: string;
  } = {} as any
): string {
  let path = originalPath;
  if (path.endsWith("*") && path !== "*" && !path.endsWith("/*")) {
    warning(
      false,
      `Route path "${path}" will be treated as if it were ` +
        `"${path.replace(/\*$/, "/*")}" because the \`*\` character must ` +
        `always follow a \`/\` in the pattern. To get rid of this warning, ` +
        `please change the route path to "${path.replace(/\*$/, "/*")}".`
    );
    path = path.replace(/\*$/, "/*") as Path;
  }

  return path
    .replace(/^:(\w+)/g, (_, key: PathParam<Path>) => {
      invariant(params[key] != null, `Missing ":${key}" param`);
      return params[key]!;
    })
    .replace(/\/:(\w+)/g, (_, key: PathParam<Path>) => {
      invariant(params[key] != null, `Missing ":${key}" param`);
      return `/${params[key]!}`;
    })
    .replace(/(\/?)\*/, (_, prefix, __, str) => {
      const star = "*" as PathParam<Path>;

      if (params[star] == null) {
        // If no splat was provided, trim the trailing slash _unless_ it's
        // the entire path
        return str === "/*" ? "/" : "";
      }

      // Apply the splat
      return `${prefix}${params[star]}`;
    });
}
Repository: remix-run/react-router
File: packages/router/utils.ts (TypeScript)

Code before:
 */
export interface PathPattern<Path extends string = string> {
  /**
   * A string to match against a URL pathname. May contain `:id`-style segments
   * to indicate placeholders for dynamic parameters. May also end with `/*` to
   * indicate matching the rest of the URL pathname.
   */
  path: Path;
  /**
   * Should be `true` if the static portions of the `path` should be matched in
   * the same case.
   */
  caseSensitive?: boolean;
  /**
   * Should be `true` if this pattern should match the entire URL pathname.
   */
  end?: boolean;
}

/**
 * A PathMatch contains info about how a PathPattern matched on a URL pathname.
 */
export interface PathMatch<ParamKey extends string = string> {
  /**
   * The names and values of dynamic parameters in the URL.
   */
  params: Params<ParamKey>;
  /**
   * The portion of the URL pathname that was matched.
   */
  pathname: string;
  /**
   * The portion of the URL pathname that was matched before child routes.
   */
  pathnameBase: string;
  /**
   * The pattern that was used to match.
   */
  pattern: PathPattern;
}

type Mutable<T> = {
  -readonly [P in keyof T]: T[P];
};

/**
 * Performs pattern matching on a URL pathname and returns information about
 * the match.
 *
 * @see https://reactrouter.com/docs/en/v6/utils/match-path
 */

Review comment (suggestion):
`generatePath` does 2 separate checks here to avoid a nasty regex to differentiate `:param` from `/thing/:param`

Code after:
  originalPath: Path,
  params: {
    [key in PathParam<Path>]: string;
  } = {} as any
): string {
  let path = originalPath;
  if (path.endsWith("*") && path !== "*" && !path.endsWith("/*")) {
    warning(
      false,
      `Route path "${path}" will be treated as if it were ` +
        `"${path.replace(/\*$/, "/*")}" because the \`*\` character must ` +
        `always follow a \`/\` in the pattern. To get rid of this warning, ` +
        `please change the route path to "${path.replace(/\*$/, "/*")}".`
    );
    path = path.replace(/\*$/, "/*") as Path;
  }

  return path
    .replace(/^:(\w+)/g, (_, key: PathParam<Path>) => {
      invariant(params[key] != null, `Missing ":${key}" param`);
      return params[key]!;
    })
    .replace(/\/:(\w+)/g, (_, key: PathParam<Path>) => {
      invariant(params[key] != null, `Missing ":${key}" param`);
      return `/${params[key]!}`;
    })
    .replace(/(\/?)\*/, (_, prefix, __, str) => {
      const star = "*" as PathParam<Path>;

      if (params[star] == null) {
        // If no splat was provided, trim the trailing slash _unless_ it's
        // the entire path
        return str === "/*" ? "/" : "";
      }

      // Apply the splat
      return `${prefix}${params[star]}`;
    });
}

/**
 * A PathPattern is used to match on some portion of a URL pathname.
 */
export interface PathPattern<Path extends string = string> {
  /**
   * A string to match against a URL pathname. May contain `:id`-style segments
   * to indicate placeholders for dynamic parameters. May also end with `/*` to
   * indicate matching the rest of the URL pathname.
   */
  path: Path;
  /**
Repository: remix-run/react-router
File: packages/react-router/__tests__/data-memory-router-test.tsx (TypeScript)

Code before:
          <Route path="to/*" element={<GrandChild />} />
        </Routes>
      );
    }

    expect(getHtml(container)).toMatchInlineSnapshot(`
      "<div>
        <h1>
          👋 Hello from the other side!
        </h1>
      </div>"
    `);
  });

  it.only("renders <Routes> alongside a data router ErrorBoundary", () => {
    let router = createMemoryRouter(
      [
        {
          path: "*",
          Component() {
            return (
              <>
                <Outlet />
                <Routes>
                  <Route index element={<h1>Descendant</h1>} />
                </Routes>
              </>
            );
          },
          children: [
            {
              id: "index",
              index: true,
              Component: () => <h1>Child</h1>,
              ErrorBoundary() {
                return <p>{(useRouteError() as Error).message}</p>;
              },
            },
          ],
        },
      ],
      {
        initialEntries: ["/"],
        hydrationData: {
          errors: {
            index: new Error("Broken!"),
          },
        },
      }
    );
    let { container } = render(<RouterProvider router={router} />);

Review comment (suggestion):
This is a bit of an unusual use-case.  Normally the presence of an error would mean we don't reach descendant `<Routes>` down inside children `Component`'s.  But if the `<Routes>` is next to the `<Outlet />` then both can render at the same time.

Code after:
          <Route path="to/*" element={<GrandChild />} />
        </Routes>
      );
    }

    expect(getHtml(container)).toMatchInlineSnapshot(`
      "<div>
        <h1>
          👋 Hello from the other side!
        </h1>
      </div>"
    `);
  });

  it("renders <Routes> alongside a data router ErrorBoundary", () => {
    let router = createMemoryRouter(
      [
        {
          path: "*",
          Component() {
            return (
              <>
                <Outlet />
                <Routes>
                  <Route index element={<h1>Descendant</h1>} />
                </Routes>
              </>
            );
          },
          children: [
            {
              id: "index",
              index: true,
              Component: () => <h1>Child</h1>,
              ErrorBoundary() {
                return <p>{(useRouteError() as Error).message}</p>;
              },
            },
          ],
        },
      ],
      {
        initialEntries: ["/"],
        hydrationData: {
          errors: {
            index: new Error("Broken!"),
          },
        },
      }
    );
    let { container } = render(<RouterProvider router={router} />);
Repository: remix-run/react-router
File: packages/react-router-dom/jest.config.js (JavaScript)

Code before:
module.exports = {
  testEnvironment: "jsdom",
  testMatch: ["**/__tests__/*-test.[jt]s?(x)"],
  transform: {
    "\\.[jt]sx?$": "./jest-transformer.js",
  },
  globals: {
    __DEV__: true,
  },
  setupFiles: ["./__tests__/setup.ts"],
  moduleNameMapper: {
    "^@remix-run/router$": "<rootDir>/../router/index.ts",
    "^@remix-run/web-blob$": require.resolve("@remix-run/web-blob"),
    "^@remix-run/web-fetch$": require.resolve("@remix-run/web-fetch"),
    "^@remix-run/web-form-data$": require.resolve("@remix-run/web-form-data"),
    "^@remix-run/web-stream$": require.resolve("@remix-run/web-stream"),
    "^@web3-storage/multipart-parser$": require.resolve(
      "@web3-storage/multipart-parser"
    ),
    "^react-router$": "<rootDir>/../react-router/index.ts",
    "^react-router-dom$": "<rootDir>/index.tsx",
  },
};


Review comment (suggestion):
explicitly setting the reporter makes it so it doesn't get set to `github-actions` which will do annotations on the "files changed" tab

https://jestjs.io/docs/configuration#reporters-arraymodulename--modulename-options

Code after:
/** @type {import('jest').Config} */
module.exports = {
  testEnvironment: "jsdom",
  testMatch: ["**/__tests__/*-test.[jt]s?(x)"],
  transform: {
    "\\.[jt]sx?$": "./jest-transformer.js",
  },
  globals: {
    __DEV__: true,
  },
  setupFiles: ["./__tests__/setup.ts"],
  moduleNameMapper: {
    "^@remix-run/router$": "<rootDir>/../router/index.ts",
    "^@remix-run/web-blob$": require.resolve("@remix-run/web-blob"),
    "^@remix-run/web-fetch$": require.resolve("@remix-run/web-fetch"),
    "^@remix-run/web-form-data$": require.resolve("@remix-run/web-form-data"),
    "^@remix-run/web-stream$": require.resolve("@remix-run/web-stream"),
    "^@web3-storage/multipart-parser$": require.resolve(
      "@web3-storage/multipart-parser"
    ),
    "^react-router$": "<rootDir>/../react-router/index.ts",
    "^react-router-dom$": "<rootDir>/index.tsx",
  },
  reporters: ["default"],
};
Repository: remix-run/react-router
File: packages/router/utils.ts (TypeScript)

Code before:

  let headers = new Headers(responseInit.headers);
  headers.set("Location", url);

  return new Response(null, {
    ...responseInit,
    headers,
  });
};

/**
 * A redirect response with a forced document reload. Sets a custom header to
 * trigger the client-side reload.
 * Defaults to "302 Found".
 */
export const redirectWithReload: RedirectFunction = (url, init = 302) => {
  let responseInit = init;
  if (typeof responseInit === "number") {
    responseInit = { status: responseInit };
  }

  responseInit.headers = new Headers(responseInit.headers);
  responseInit.headers.set("X-Remix-Reload-Document", "true");

  return redirect(url, responseInit);
};

/**
 * @private
 * Utility class we use to hold auto-unwrapped 4xx/5xx Response bodies
 */
export class ErrorResponse {
  status: number;
  statusText: string;
  data: any;
  error?: Error;
  internal: boolean;

  constructor(
    status: number,
    statusText: string | undefined,
    data: any,
    internal = false
  ) {
    this.status = status;
    this.statusText = statusText || "";
    this.internal = internal;
    if (data instanceof Error) {
      this.data = data.toString();
      this.error = data;
    } else {

Review comment (suggestion):
Could we build this around `redirect`?

```suggestion
export const redirectWithReload: RedirectFunction = (url, init) => {
  let response = redirect(url, init);
  response.headers.set("X-Remix-Reload-Document", "true");
  return response;
};
```

Code after:

  let headers = new Headers(responseInit.headers);
  headers.set("Location", url);

  return new Response(null, {
    ...responseInit,
    headers,
  });
};

/**
 * A redirect response that will force a document reload to the new location.
 * Sets the status code and the `Location` header.
 * Defaults to "302 Found".
 */
export const redirectDocument: RedirectFunction = (url, init) => {
  let response = redirect(url, init);
  response.headers.set("X-Remix-Reload-Document", "true");
  return response;
};

/**
 * @private
 * Utility class we use to hold auto-unwrapped 4xx/5xx Response bodies
 */
export class ErrorResponse {
  status: number;
  statusText: string;
  data: any;
  error?: Error;
  internal: boolean;

  constructor(
    status: number,
    statusText: string | undefined,
    data: any,
    internal = false
  ) {
    this.status = status;
    this.statusText = statusText || "";
    this.internal = internal;
    if (data instanceof Error) {
      this.data = data.toString();
      this.error = data;
    } else {
      this.data = data;
    }
  }
}

/**
Repository: remix-run/react-router
File: packages/react-router-dom/server.tsx (TypeScript)

Code before:
    deleteFetcher() {
      throw msg("deleteFetcher");
    },
    dispose() {
      throw msg("dispose");
    },
    getBlocker() {
      return IDLE_BLOCKER;
    },
    deleteBlocker() {
      throw msg("deleteBlocker");
    },
    _internalFetchControllers: new Map(),
    _internalActiveDeferreds: new Map(),
    _internalSetRoutes() {
      throw msg("_internalSetRoutes");
    },
  };
}

function createHref(to: To) {
  return typeof to === "string" ? to : createPath(to);
}

function encodeLocation(to: To): Path {
  // Locations should already be encoded on the server, so just return as-is
  let path = typeof to === "string" ? parsePath(to) : to;
  return {
    pathname: path.pathname || "",
    search: path.search || "",
    hash: path.hash || "",
  };
}

// This utility is based on https://github.com/zertosh/htmlescape
// License: https://github.com/zertosh/htmlescape/blob/0527ca7156a524d256101bb310a9f970f63078ad/LICENSE
const ESCAPE_LOOKUP: { [match: string]: string } = {
  "&": "\\u0026",
  ">": "\\u003e",
  "<": "\\u003c",
  "\u2028": "\\u2028",
  "\u2029": "\\u2029",
};

const ESCAPE_REGEX = /[&><\u2028\u2029]/g;

function htmlEscape(str: string): string {
  return str.replace(ESCAPE_REGEX, (match) => ESCAPE_LOOKUP[match]);
}


Review comment (suggestion):
This was a bad assumption - while they're properly encoded in `request.url`, we decode paths for matching and this function is used to re-encode in external-facing paths.

Code after:
    deleteFetcher() {
      throw msg("deleteFetcher");
    },
    dispose() {
      throw msg("dispose");
    },
    getBlocker() {
      return IDLE_BLOCKER;
    },
    deleteBlocker() {
      throw msg("deleteBlocker");
    },
    _internalFetchControllers: new Map(),
    _internalActiveDeferreds: new Map(),
    _internalSetRoutes() {
      throw msg("_internalSetRoutes");
    },
  };
}

function createHref(to: To) {
  return typeof to === "string" ? to : createPath(to);
}

function encodeLocation(to: To): Path {
  let href = typeof to === "string" ? to : createPath(to);
  let encoded = ABSOLUTE_URL_REGEX.test(href)
    ? new URL(href)
    : new URL(href, "http://localhost");
  return {
    pathname: encoded.pathname,
    search: encoded.search,
    hash: encoded.hash,
  };
}

const ABSOLUTE_URL_REGEX = /^(?:[a-z][a-z0-9+.-]*:|\/\/)/i;

// This utility is based on https://github.com/zertosh/htmlescape
// License: https://github.com/zertosh/htmlescape/blob/0527ca7156a524d256101bb310a9f970f63078ad/LICENSE
const ESCAPE_LOOKUP: { [match: string]: string } = {
  "&": "\\u0026",
  ">": "\\u003e",
  "<": "\\u003c",
  "\u2028": "\\u2028",
  "\u2029": "\\u2029",
};

const ESCAPE_REGEX = /[&><\u2028\u2029]/g;

function htmlEscape(str: string): string {
Repository: remix-run/react-router
File: packages/router/utils.ts (TypeScript)

Code before:
    ...responseInit,
    headers,
  });
};

/**
 * A redirect response that will force a document reload to the new location.
 * Sets the status code and the `Location` header.
 * Defaults to "302 Found".
 */
export const redirectDocument: RedirectFunction = (url, init) => {
  let response = redirect(url, init);
  response.headers.set("X-Remix-Reload-Document", "true");
  return response;
};

/**
 * @private
 * Utility class we use to hold auto-unwrapped 4xx/5xx Response bodies
 */
export class ErrorResponse {
  status: number;
  statusText: string;
  data: any;
  error?: Error;
  internal: boolean;

  constructor(
    status: number,
    statusText: string | undefined,
    data: any,
    internal = false
  ) {
    this.status = status;
    this.statusText = statusText || "";
    this.internal = internal;
    if (data instanceof Error) {
      this.data = data.toString();
      this.error = data;
    } else {
      this.data = data;
    }
  }
}

/**
 * Check if the given error is an ErrorResponse generated from a 4xx/5xx
 * Response thrown from an action/loader
 */
export function isRouteErrorResponse(error: any): error is ErrorResponse {
  return (

Review comment (suggestion):
These should have been private all along, but since the entire class was a "private" implementation detail I think I glossed over the field specifiers.  Marking them private ensures the type below doesn't include them.

Code after:
    ...responseInit,
    headers,
  });
};

/**
 * A redirect response that will force a document reload to the new location.
 * Sets the status code and the `Location` header.
 * Defaults to "302 Found".
 */
export const redirectDocument: RedirectFunction = (url, init) => {
  let response = redirect(url, init);
  response.headers.set("X-Remix-Reload-Document", "true");
  return response;
};

/**
 * @private
 * Utility class we use to hold auto-unwrapped 4xx/5xx Response bodies
 */
export class ErrorResponseImpl {
  status: number;
  statusText: string;
  data: any;
  private error?: Error;
  private internal: boolean;

  constructor(
    status: number,
    statusText: string | undefined,
    data: any,
    internal = false
  ) {
    this.status = status;
    this.statusText = statusText || "";
    this.internal = internal;
    if (data instanceof Error) {
      this.data = data.toString();
      this.error = data;
    } else {
      this.data = data;
    }
  }
}

// We don't want the class exported since usage of it at runtime is an
// implementation detail, but we do want to export the shape so folks can
// build their own abstractions around instances via isRouteErrorResponse()
export type ErrorResponse = InstanceType<typeof ErrorResponseImpl>;

/**