ronantakizawa/codereview-bench huggingface dataset:
Repository: electron-userland/electron-builder
File: packages/app-builder-lib/src/util/yarn.ts (TypeScript)

Code before:

function isRunningYarn(execPath: string | null | undefined) {
  const userAgent = process.env.npm_config_user_agent
  return process.env.FORCE_YARN === "true" || (execPath != null && path.basename(execPath).startsWith("yarn")) || (userAgent != null && /\byarn\b/.test(userAgent))
}

export interface RebuildOptions {
  frameworkInfo: DesktopFrameworkInfo

  platform?: NodeJS.Platform
  arch?: string

  buildFromSource?: boolean

  additionalArgs?: Array<string> | null
}

/** @internal */
export async function rebuild(appDir: string, buildFromSource: boolean, frameworkInfo: DesktopFrameworkInfo, arch: Arch) {
  log.info({ arch, version: frameworkInfo.version, appDir }, "executing @electron/rebuild")
  const rootPath = await searchModule.getProjectRootPath(appDir)
  const options: electronRebuild.RebuildOptions = {
    buildPath: appDir,
    electronVersion: frameworkInfo.version,
    arch: Arch[arch],
    projectRootPath: rootPath,
    disablePreGypCopy: true,
  }
  if (buildFromSource) {
    options.prebuildTagPrefix = "totally-not-a-real-prefix-to-force-rebuild"
  }
  return electronRebuild.rebuild(options)
}


Review comment (performance):
Can you also add a different `cachePath` argument for different arch? 

https://github.com/electron/rebuild/blob/main/src/types.ts#L10

Code after:

function isRunningYarn(execPath: string | null | undefined) {
  const userAgent = process.env.npm_config_user_agent
  return process.env.FORCE_YARN === "true" || (execPath != null && path.basename(execPath).startsWith("yarn")) || (userAgent != null && /\byarn\b/.test(userAgent))
}

export interface RebuildOptions {
  frameworkInfo: DesktopFrameworkInfo

  platform?: NodeJS.Platform
  arch?: string

  buildFromSource?: boolean

  additionalArgs?: Array<string> | null
}

/** @internal */
export async function rebuild(appDir: string, buildFromSource: boolean, frameworkInfo: DesktopFrameworkInfo, arch: Arch) {
  log.info({ arch: Arch[arch], version: frameworkInfo.version, appDir }, "executing @electron/rebuild")
  const rootPath = await searchModule.getProjectRootPath(appDir)
  const options: electronRebuild.RebuildOptions = {
    buildPath: appDir,
    electronVersion: frameworkInfo.version,
    arch: Arch[arch],
    projectRootPath: rootPath,
    disablePreGypCopy: true,
  }
  if (buildFromSource) {
    options.prebuildTagPrefix = "totally-not-a-real-prefix-to-force-rebuild"
  }
  return electronRebuild.rebuild(options)
}
Repository: electron-userland/electron-builder
File: packages/app-builder-lib/src/util/filename.ts (TypeScript)

Code before:
// @ts-ignore
import * as _sanitizeFileName from "sanitize-filename"
import * as path from "path"

export function sanitizeFileName(s: string): string {
  return _sanitizeFileName(s)
}

// Get the filetype from a filename. Returns a string of one or more file extensions,
// e.g. .zip, .dmg, .tar.gz, .tar.bz2, .exe.blockmap. We'd normally use `path.extname()`,
// but it doesn't support multiple extensions, e.g. Foo-1.0.0.dmg.blockmap should be
// .dmg.blockmap, not .blockmap.
export function getCompleteExtname(filename: string): string {
  let extname = path.extname(filename)

  switch (extname) {
    // Append leading extension for blockmap filetype
    case ".blockmap": {
      extname = path.extname(filename.replace(extname, "")) + extname

      break
    }
    // Append leading extension for known compressed tar formats
    case ".bz2":
    case ".gz":
    case ".lz":
    case ".xz":
    case ".7z": {
      const ext = path.extname(filename.replace(extname, ""))
      if (ext === ".tar") {
        extname = ext + extname
      }

Review comment (security):
What is `normalize("NFD")`? I don't see it in the API or type definitions: https://github.com/parshap/node-sanitize-filename/blob/master/index.d.ts

Code after:
// @ts-ignore
import * as _sanitizeFileName from "sanitize-filename"
import * as path from "path"

export function sanitizeFileName(s: string, normalizeNfd = false): string {
  const sanitized = _sanitizeFileName(s)
  return normalizeNfd ? sanitized.normalize("NFD") : sanitized
}

// Get the filetype from a filename. Returns a string of one or more file extensions,
// e.g. .zip, .dmg, .tar.gz, .tar.bz2, .exe.blockmap. We'd normally use `path.extname()`,
// but it doesn't support multiple extensions, e.g. Foo-1.0.0.dmg.blockmap should be
// .dmg.blockmap, not .blockmap.
export function getCompleteExtname(filename: string): string {
  let extname = path.extname(filename)

  switch (extname) {
    // Append leading extension for blockmap filetype
    case ".blockmap": {
      extname = path.extname(filename.replace(extname, "")) + extname

      break
    }
    // Append leading extension for known compressed tar formats
    case ".bz2":
    case ".gz":
    case ".lz":
    case ".xz":
    case ".7z": {
      const ext = path.extname(filename.replace(extname, ""))
      if (ext === ".tar") {
        extname = ext + extname
Repository: electron-userland/electron-builder
File: packages/builder-util-runtime/src/publishOptions.ts (TypeScript)

Code before:

  /**
   * The channel.
   * @default latest
   */
  readonly channel?: string | null

  /**
   * Whether to use multiple range requests for differential update. Defaults to `true` if `url` doesn't contain `s3.amazonaws.com`.
   */
  readonly useMultipleRangeRequest?: boolean
}

/**
 * Keygen options.
 * https://keygen.sh/
 * Define `KEYGEN_TOKEN` environment variable.
 */
export interface KeygenOptions extends PublishConfiguration {
  /**
   * The provider. Must be `keygen`.
   */
  readonly provider: "keygen"

  /**
   * Keygen host
   */
  readonly host?: string

  /**
   * Keygen account's UUID
   */
  readonly account: string

  /**
   * Keygen product's UUID
   */
  readonly product: string

  /**
   * The channel.
   * @default stable
   */
  readonly channel?: "stable" | "rc" | "beta" | "alpha" | "dev" | null

  /**
   * The target Platform. Is set programmatically explicitly during publishing.
   */
  readonly platform?: string | null
}


Review comment (suggestion):
Let's add more details to this documentation with a default value.
```
Keygen host for self-hosted instances
@default "api.keygen.sh"
```

Code after:

  /**
   * The channel.
   * @default latest
   */
  readonly channel?: string | null

  /**
   * Whether to use multiple range requests for differential update. Defaults to `true` if `url` doesn't contain `s3.amazonaws.com`.
   */
  readonly useMultipleRangeRequest?: boolean
}

/**
 * Keygen options.
 * https://keygen.sh/
 * Define `KEYGEN_TOKEN` environment variable.
 */
export interface KeygenOptions extends PublishConfiguration {
  /**
   * The provider. Must be `keygen`.
   */
  readonly provider: "keygen"

  /**
   * Keygen host for self-hosted instances
   * @default "api.keygen.sh"
   */
  readonly host?: string

  /**
   * Keygen account's UUID
   */
  readonly account: string

  /**
   * Keygen product's UUID
   */
  readonly product: string

  /**
   * The channel.
   * @default stable
   */
  readonly channel?: "stable" | "rc" | "beta" | "alpha" | "dev" | null

  /**
   * The target Platform. Is set programmatically explicitly during publishing.
   */
  readonly platform?: string | null
}
Repository: electron-userland/electron-builder
File: packages/electron-updater/src/RpmUpdater.ts (TypeScript)

Code before:
  }

  protected doInstall(options: InstallOptions): boolean {
    try {
      const sudo = this.wrapSudo()
      // pkexec doesn't want the command to be wrapped in " quotes
      const wrapper = /pkexec/i.test(sudo) ? "" : `"`
      const packageManager = this.spawnSyncLog("which zypper")
      const installerPath = this.installerPath
      if (installerPath == null) {
        this.dispatchError(new Error("No valid update available, can't quit and install"))
        return false
      }
      let cmd: string[]
      if (!packageManager) {
        const packageManager = this.spawnSyncLog("which dnf || which yum")
        cmd = [packageManager, "-y", "install", installerPath]
      } else {
        cmd = [packageManager, "--no-refresh", "install", "--allow-unsigned-rpm", "-y", "-f", installerPath]
      }
      this.spawnSyncLog(sudo, [`${wrapper}/bin/bash`, "-c", `'${cmd.join(" ")}'${wrapper}`])
      if (options.isForceRunAfter) {
        this.app.relaunch()
      }
      return true
    } catch (e) {
      this.dispatchError(e as Error)
      return false
    }
  }
}


Review comment (suggestion):
Looks like we don't need to wrap this with try-catch as it's already done on in the call to `doInstall`
https://github.com/electron-userland/electron-builder/blob/bee179b3cf8163041d280ed8dc5a5ce4f27786c6/packages/electron-updater/src/BaseUpdater.ts#L65-L75

I'll push an update to this PR

Code after:
  }

  protected doInstall(options: InstallOptions): boolean {
    const sudo = this.wrapSudo()
    // pkexec doesn't want the command to be wrapped in " quotes
    const wrapper = /pkexec/i.test(sudo) ? "" : `"`
    const packageManager = this.spawnSyncLog("which zypper")
    const installerPath = this.installerPath
    if (installerPath == null) {
      this.dispatchError(new Error("No valid update available, can't quit and install"))
      return false
    }
    let cmd: string[]
    if (!packageManager) {
      const packageManager = this.spawnSyncLog("which dnf || which yum")
      cmd = [packageManager, "-y", "install", installerPath]
    } else {
      cmd = [packageManager, "--no-refresh", "install", "--allow-unsigned-rpm", "-y", "-f", installerPath]
    }
    this.spawnSyncLog(sudo, [`${wrapper}/bin/bash`, "-c", `'${cmd.join(" ")}'${wrapper}`])
    if (options.isForceRunAfter) {
      this.app.relaunch()
    }
    return true
  }
}
Repository: electron-userland/electron-builder
File: packages/app-builder-lib/src/node-module-collector/types.ts (TypeScript)

Code before:
  dependencies?: Array<NodeModuleInfo>
}

export interface ParsedDependencyTree {
  readonly name: string
  readonly version: string
  readonly path: string
  readonly workspaces?: string[] // we only use this at root level
}

export interface DependencyTree extends Dependency<DependencyTree>, ParsedDependencyTree {
  implicitDependenciesInjected: boolean
}

export interface NpmDependency extends Dependency<NpmDependency>, ParsedDependencyTree {
  // Output of `JSON.parse(...)` of `npm list` and `pnpm list`
  // This object has a TON of info - a majority, if not all, of the dependency's package.json
  // We extract only what we need when constructing DependencyTree
}

export type Dependency<T> = {
  dependencies?: {
    [packageName: string]: T
  }
  optionalDependencies?: {
    [packageName: string]: string
  }
  peerDependencies?: {
    [packageName: string]: string
  }
  // npm-specific: implicit dependencies from `npm list --json`
  _dependencies?: {
    [packageName: string]: string
  }
}

export interface DependencyGraph {
  [packageNameAndVersion: string]: PackageDependencies
}

interface PackageDependencies {
  dependencies: string[]
}


Review comment (suggestion):
This is not a string type; it's still a DependencyTree.


```
optionalDependencies?: {
    [packageName: string]: T
}
```

Code after:
  dependencies?: Array<NodeModuleInfo>
}

export type ParsedDependencyTree = {
  readonly name: string
  readonly version: string
  readonly path: string
  readonly workspaces?: string[] // we only use this at root level
}

export interface DependencyTree extends Omit<Dependency<DependencyTree, DependencyTree>, "optionalDependencies"> {
  readonly implicitDependenciesInjected: boolean
}

// Note: `PnpmDependency` and `NpmDependency` include the output of `JSON.parse(...)` of `pnpm list` and `npm list` respectively
// This object has a TON of info - a majority, if not all, of each dependency's package.json
// We extract only what we need when constructing DependencyTree in `extractProductionDependencyTree`
// eslint-disable-next-line @typescript-eslint/no-empty-object-type
export interface PnpmDependency extends Dependency<PnpmDependency, PnpmDependency> {}
export interface NpmDependency extends Dependency<NpmDependency, string> {
  // implicit dependencies
  readonly _dependencies?: {
    [packageName: string]: string
  }
}

export type Dependency<T, V> = Dependencies<T, V> & ParsedDependencyTree

export type Dependencies<T, V> = {
  readonly dependencies?: {
    [packageName: string]: T
  }
  readonly optionalDependencies?: {
    [packageName: string]: V
  }
}

export interface DependencyGraph {
  [packageNameAndVersion: string]: PackageDependencies
}

interface PackageDependencies {
  readonly dependencies: string[]
}
Repository: electron-userland/electron-builder
File: packages/app-builder-lib/src/util/plist.ts (TypeScript)

Code before:
    return obj
  }

  if (Array.isArray(obj)) {
    return obj.map(sortObjectKeys)
  }

  const result: PlistObject = {}
  Object.keys(obj)
    .sort()
    .forEach(key => {
      result[key] = sortObjectKeys((obj as PlistObject)[key])
    })
  return result
}

export async function savePlistFile(path: string, data: PlistValue): Promise<void> {
  const sortedData = sortObjectKeys(data)
  const plist = build(sortedData)
  await fs.promises.writeFile(path, plist)
}

export async function parsePlistFile<T>(file: string): Promise<T> {
  const data = await fs.promises.readFile(file, "utf8")
  return parse(data) as T
}

export type { PlistValue, PlistObject }


Review comment (suggestion):
nit: Can we import `import * as fs from "fs/promises"` instead of accessing it directly through `fs.promises`? (It'll help me later when I need to regex find+migrate from fs-extra to `fs/promises` (where we're able to at least)

Code after:
    return obj
  }

  if (Array.isArray(obj)) {
    return obj.map(sortObjectKeys)
  }

  const result: PlistObject = {}
  Object.keys(obj)
    .sort()
    .forEach(key => {
      result[key] = sortObjectKeys((obj as PlistObject)[key])
    })
  return result
}

export async function savePlistFile(path: string, data: PlistValue): Promise<void> {
  const sortedData = sortObjectKeys(data)
  const plist = build(sortedData)
  await fs.writeFile(path, plist)
}

export async function parsePlistFile<T>(file: string): Promise<T> {
  const data = await fs.readFile(file, "utf8")
  return parse(data) as T
}

export type { PlistValue, PlistObject }
Repository: electron-userland/electron-builder
File: packages/app-builder-lib/src/node-module-collector/packageManager.ts (TypeScript)

Code before:
  if (globalInstallationCache.has(key)) {
    return Promise.resolve(globalInstallationCache.get(key)!)
  }

  return exec(pm, ["--version"], { shell: true })
    .then(res => {
      return /^\d+.\d+.\d+$/.test(res)
    })
    .then(value => {
      globalInstallationCache.set(key, value)
      return value
    })
    .catch(() => false)
}

function getTypeofLockFile(cwd = process.cwd()): Promise<PM> {
  const key = `lockfile_${cwd}`
  if (lockfileCache.has(key)) {
    return Promise.resolve(lockfileCache.get(key)!)
  }

  return Promise.all([
    exists(resolve(cwd, "yarn.lock")),
    exists(resolve(cwd, "package-lock.json")),
    exists(resolve(cwd, "pnpm-lock.yaml")),
    exists(resolve(cwd, "bun.lockb")),
  ]).then(([isYarn, _, isPnpm, isBun]) => {
    let value: PM

    if (isYarn) {
      value = "yarn"
    } else if (isPnpm) {
      value = "pnpm"
    } else if (isBun) {
      value = "bun"
    } else {
      value = "npm"
    }

    cache.set(key, value)
    return value
  })
}

export const detect = async ({ cwd, includeGlobalBun }: { cwd?: string; includeGlobalBun?: boolean } = {}) => {
  let type = await getTypeofLockFile(cwd)
  if (type) {
    return type
  }

  let tmpCwd = cwd || "."

Review comment (suggestion):
Bun 1.2 changed the default lockfile format to text-based, so the extension is `.lock` and not just `.lockb`. I think the binary version `.lockb` is still supported, just not the default anymore, so there should be a check here for both

https://bun.com/blog/bun-lock-text-lockfile

Code after:
    }

    if (userAgent.includes("yarn")) {
      if (userAgent.includes("yarn/")) {
        const version = userAgent.match(/yarn\/(\d+)\./)
        if (version && parseInt(version[1]) >= 2) {
          return PM.YARN_BERRY
        }
      }
      return PM.YARN
    }

    if (userAgent.includes("npm")) {
      return PM.NPM
    }
  }

  if (process.env.npm_execpath) {
    const execPath = process.env.npm_execpath.toLowerCase()

    if (execPath.includes("pnpm")) {
      return PM.PNPM
    }

    if (execPath.includes("yarn")) {
      if (execPath.includes("berry") || process.env.YARN_VERSION?.startsWith("2.") || process.env.YARN_VERSION?.startsWith("3.")) {
        return PM.YARN_BERRY
      }
      return PM.YARN
    }

    if (execPath.includes("npm")) {
      return PM.NPM
    }
  }

  if (process.env.PNPM_HOME) {
    return PM.PNPM
  }

  if (process.env.YARN_REGISTRY) {
    if (process.env.YARN_VERSION?.startsWith("2.") || process.env.YARN_VERSION?.startsWith("3.")) {
      return PM.YARN_BERRY
    }
    return PM.YARN
  }

  if (process.env.npm_package_json) {
    return PM.NPM
  }
Repository: electron-userland/electron-builder
File: packages/builder-util-runtime/src/publishOptions.ts (TypeScript)

Code before:
 *
 * GitLab [personal access token](https://docs.gitlab.com/ee/user/profile/personal_access_tokens.html) is required for private repositories. You can generate one by going to your GitLab profile settings.
 * Define `GITLAB_TOKEN` environment variable.
 */
export interface GitlabOptions extends PublishConfiguration {
  /**
   * The provider. Must be `gitlab`.
   */
  readonly provider: "gitlab"

  /**
   * The GitLab project ID or namespace/project-name.
   */
  readonly projectId: string | number

  /**
   * The host (including the port if needed).
   * @default gitlab.com
   */
  readonly host?: string | null

  /**
   * The protocol. GitLab supports both `https` and `http`.
   * @default https
   */
  readonly protocol?: "https" | "http" | null

  /**
   * The access token to support auto-update from private GitLab repositories. Never specify it in the configuration files. Only for [setFeedURL](./auto-update.md#appupdatersetfeedurloptions).
   */
  readonly token?: string | null

  /**
   * The channel.
   * @default latest
   */
  readonly channel?: string | null
}

/**
 * Generic (any HTTP(S) server) options.
 * In all publish options [File Macros](./file-patterns.md#file-macros) are supported.
 */
export interface GenericServerOptions extends PublishConfiguration {
  /**
   * The provider. Must be `generic`.
   */
  readonly provider: "generic"

  /**
   * The base url. e.g. `https://bucket_name.s3.amazonaws.com`.

Review comment (security):
For the sake of security, do we need to allow an `http` option? Is there a usecase for it?

Code after:
 *
 * GitLab [personal access token](https://docs.gitlab.com/ee/user/profile/personal_access_tokens.html) is required for private repositories. You can generate one by going to your GitLab profile settings.
 * Define `GITLAB_TOKEN` environment variable.
 */
export interface GitlabOptions extends PublishConfiguration {
  /**
   * The provider. Must be `gitlab`.
   */
  readonly provider: "gitlab"

  /**
   * The GitLab project ID or namespace/project-name.
   */
  readonly projectId: string | number

  /**
   * The host (including the port if needed).
   * @default gitlab.com
   */
  readonly host?: string | null

  /**
   * The access token to support auto-update from private GitLab repositories. Never specify it in the configuration files. Only for [setFeedURL](./auto-update.md#appupdatersetfeedurloptions).
   */
  readonly token?: string | null

  /**
   * The channel.
   * @default latest
   */
  readonly channel?: string | null
}

/**
 * Generic (any HTTP(S) server) options.
 * In all publish options [File Macros](./file-patterns.md#file-macros) are supported.
 */
export interface GenericServerOptions extends PublishConfiguration {
  /**
   * The provider. Must be `generic`.
   */
  readonly provider: "generic"

  /**
   * The base url. e.g. `https://bucket_name.s3.amazonaws.com`.
   */
  readonly url: string

  /**
   * The channel.
   * @default latest
Repository: electron-userland/electron-builder
File: packages/electron-updater/src/providers/GitLabProvider.ts (TypeScript)

Code before:

    // Create assets map from GitLab release assets
    const assetsMap = new Map<string, string>()
    for (const asset of latestRelease.assets.links) {
      assetsMap.set(asset.name, asset.direct_asset_url)
    }

    return {
      tag: tag,
      assets: assetsMap,
      ...result,
    }
  }

  resolveFiles(updateInfo: GitlabUpdateInfo): Array<ResolvedUpdateFileInfo> {
    return getFileList(updateInfo).map((fileInfo: UpdateFileInfo) => {
      // GitLab assets may have spaces replaced with dashes
      const possibleNames = [
        fileInfo.url, // Original filename
        fileInfo.url.replace(/ /g, "-"), // Spaces replaced with dashes
      ]

      let assetUrl: string | undefined
      for (const name of possibleNames) {
        assetUrl = updateInfo.assets.get(name)
        if (assetUrl) break
      }

      if (!assetUrl) {
        throw newError(
          `Cannot find asset "${fileInfo.url}" in GitLab release assets. Available assets: ${Array.from(updateInfo.assets.keys()).join(", ")}`,
          "ERR_UPDATER_ASSET_NOT_FOUND"
        )
      }

      return {
        url: new URL(assetUrl),
        info: fileInfo,
      }
    })
  }

  toString(): string {
    return `GitLab (projectId: ${this.options.projectId}, channel: ${this.channel})`
  }
}


Review comment (refactor):
Minor: Please use multi-line `if`
```
if (assetUrl) {
   return
}
```

That being said, it looks like you can simplify this to a `find`?
```
const assetUrl = Array.from(updateInfo.assets).find(asset => possibleNames.includes(asset))
```

Code after:
    }

    // Create assets map from GitLab release assets
    const assetsMap = new Map<string, string>()
    for (const asset of latestRelease.assets.links) {
      assetsMap.set(asset.name.replace(/ /g, "-"), asset.direct_asset_url)
    }

    return {
      tag: tag,
      assets: assetsMap,
      ...result,
    }
  }

  resolveFiles(updateInfo: GitlabUpdateInfo): Array<ResolvedUpdateFileInfo> {
    return getFileList(updateInfo).map((fileInfo: UpdateFileInfo) => {
      // GitLab assets may have spaces replaced with dashes
      const possibleNames = [
        fileInfo.url, // Original filename
        fileInfo.url.replace(/ /g, "-"), // Spaces replaced with dashes
      ]

      const matchingAssetName = possibleNames.find(name => updateInfo.assets.has(name))
      const assetUrl = matchingAssetName ? updateInfo.assets.get(matchingAssetName) : undefined

      if (!assetUrl) {
        throw newError(
          `Cannot find asset "${fileInfo.url}" in GitLab release assets. Available assets: ${Array.from(updateInfo.assets.keys()).join(", ")}`,
          "ERR_UPDATER_ASSET_NOT_FOUND"
        )
      }

      return {
        url: new URL(assetUrl),
        info: fileInfo,
      }
    })
  }

  toString(): string {
    return `GitLab (projectId: ${this.options.projectId}, channel: ${this.channel})`
  }
}