ronantakizawa/codereview-bench huggingface dataset:
Repository: apple/pkl
File: pkl-cli/src/main/kotlin/org/pkl/cli/CliFormatterCommand.kt (Kotlin)

Code before:
abstract class CliFormatterCommand
@JvmOverloads
constructor(
  options: CliBaseOptions,
  protected val path: Path,
  protected val consoleWriter: Writer = System.out.writer(),
) : CliCommand(options) {
  protected fun format(file: Path, contents: String): Pair<String, Int> {
    try {
      return Formatter().format(contents) to 0
    } catch (pe: GenericParserError) {
      consoleWriter.write("Could not format `$file`: $pe")
      consoleWriter.appendLine()
      consoleWriter.flush()
      return "" to 1
    }
  }

  @OptIn(ExperimentalPathApi::class)
  protected fun paths(): Sequence<Path> {
    return if (path.isDirectory()) {
      path.walk().filter { it.extension == "pkl" || it.name == "PklProject" }
    } else sequenceOf(path)
  }
}


Review comment (bug):
Stupid question, shouldn't the filter be applied here as well? 🤔 And if it's not the case, failing with a good error message? 

Code after:
abstract class CliFormatterCommand
@JvmOverloads
constructor(
  options: CliBaseOptions,
  protected val paths: List<Path>,
  protected val consoleWriter: Writer = System.out.writer(),
) : CliCommand(options) {
  protected fun format(file: Path, contents: String): Pair<String, Int> {
    try {
      return Formatter().format(contents) to 0
    } catch (pe: GenericParserError) {
      consoleWriter.write("Could not format `$file`: $pe")
      consoleWriter.appendLine()
      consoleWriter.flush()
      return "" to 1
    }
  }

  @OptIn(ExperimentalPathApi::class)
  protected fun paths(): Set<Path> {
    val allPaths = mutableSetOf<Path>()
    for (path in paths) {
      if (path.isDirectory()) {
        allPaths.addAll(path.walk().filter { it.extension == "pkl" || it.name == "PklProject" })
      } else {
        allPaths.add(path)
      }
    }
    return allPaths
  }
}
Repository: apple/pkl
File: pkl-cli/src/main/kotlin/org/pkl/cli/CliFormatterCommand.kt (Kotlin)

Code before:
import org.pkl.commons.cli.CliBaseOptions
import org.pkl.commons.cli.CliCommand
import org.pkl.formatter.Formatter
import org.pkl.parser.GenericParserError

abstract class CliFormatterCommand
@JvmOverloads
constructor(
  options: CliBaseOptions,
  protected val path: Path,
  protected val consoleWriter: Writer = System.out.writer(),
) : CliCommand(options) {
  protected fun format(file: Path, contents: String): Pair<String, Int> {
    try {
      return Formatter().format(contents) to 0
    } catch (pe: GenericParserError) {
      consoleWriter.write("Could not format `$file`: $pe")
      consoleWriter.appendLine()
      consoleWriter.flush()
      return "" to 1
    }
  }

  @OptIn(ExperimentalPathApi::class)
  protected fun paths(): Sequence<Path> {
    return if (path.isDirectory()) {
      path.walk().filter { it.extension == "pkl" || it.name == "PklProject" }
    } else sequenceOf(path)
  }
}


Review comment (suggestion):
A bit more idiomatic kotlin would be either using `= buildSet` or `paths.map`. The current code looks a bit Java-ish 😅

Code after:
import org.pkl.commons.cli.CliBaseOptions
import org.pkl.commons.cli.CliCommand
import org.pkl.formatter.Formatter
import org.pkl.parser.GenericParserError

abstract class CliFormatterCommand
@JvmOverloads
constructor(
  options: CliBaseOptions,
  protected val paths: List<Path>,
  protected val consoleWriter: Writer = System.out.writer(),
) : CliCommand(options) {
  protected fun format(file: Path, contents: String): Pair<String, Int> {
    try {
      return Formatter().format(contents) to 0
    } catch (pe: GenericParserError) {
      consoleWriter.write("Could not format `$file`: $pe")
      consoleWriter.appendLine()
      consoleWriter.flush()
      return "" to 1
    }
  }

  @OptIn(ExperimentalPathApi::class)
  protected fun paths(): Set<Path> {
    val allPaths = mutableSetOf<Path>()
    for (path in paths) {
      if (path.isDirectory()) {
        allPaths.addAll(path.walk().filter { it.extension == "pkl" || it.name == "PklProject" })
      } else {
        allPaths.add(path)
      }
    }
    return allPaths
  }
}
Repository: apple/pkl
File: pkl-cli/src/main/kotlin/org/pkl/cli/commands/FormatterCommand.kt (Kotlin)

Code before:

  val paths: List<Path> by
    argument(name = "paths", help = "Files or directory to check.")
      .path(mustExist = true, canBeDir = true)
      .multiple()

  val grammarVersion: GrammarVersion by
    option(
        names = arrayOf("--grammar-version"),
        help =
          """
          The grammar compatibility version to use.$NEWLINE
          ${GrammarVersion.entries.joinToString("$NEWLINE", prefix = "  ") {
            val default = if (it == GrammarVersion.latest()) " `(default)`" else ""
            "`${it.version}`: ${it.versionSpan}$default"
          }}
          """
            .trimIndent(),
      )
      .enum<GrammarVersion> { "${it.version}" }
      .default(GrammarVersion.latest())

  val overwrite: Boolean by
    option(
        names = arrayOf("-w", "--write"),
        help = "Format files in place, overwriting them. Implies `--names`",
      )
      .flag(default = false)

  val names: Boolean by
    option(
        names = arrayOf("--names"),
        help = "Write the path of files with formatting violations to stdout",
      )
      .flag(default = false)

  override fun run() {
    CliFormatterCommand(paths, grammarVersion, overwrite, names).run()
  }
}


Review comment (suggestion):
```suggestion
        help = "Format files in place, overwriting them. Implies `--names`.",
```

Code after:
    argument(name = "paths", help = "Files or directory to check. Use `-` to read from stdin.")
      .path(mustExist = false, canBeDir = true)
      .multiple()

  val grammarVersion: GrammarVersion by
    option(
        names = arrayOf("--grammar-version"),
        help =
          """
          The grammar compatibility version to use.$NEWLINE
          ${GrammarVersion.entries.joinToString("$NEWLINE", prefix = "  ") {
            val default = if (it == GrammarVersion.latest()) " `(default)`" else ""
            "`${it.version}`: ${it.versionSpan}$default"
          }}
          """
            .trimIndent(),
      )
      .enum<GrammarVersion> { "${it.version}" }
      .default(GrammarVersion.latest())

  val overwrite: Boolean by
    option(
        names = arrayOf("-w", "--write"),
        help = "Format files in place, overwriting them. Implies `---diff-name-only`.",
      )
      .flag(default = false)

  val diffNameOnly: Boolean by
    option(
        names = arrayOf("--diff-name-only"),
        help = "Write the path of files with formatting violations to stdout.",
      )
      .flag(default = false)

  val silent: Boolean by
    option(
        names = arrayOf("-s", "--silent"),
        help = "Don't write to stdout or stderr. Mutually exclusive with `--diff-name-only`.",
      )
      .flag(default = false)

  override fun run() {
    CliFormatterCommand(paths, grammarVersion, overwrite, diffNameOnly, silent).run()
  }
}
Repository: apple/pkl
File: pkl-cli/src/main/kotlin/org/pkl/cli/commands/FormatterCommand.kt (Kotlin)

Code before:
    option(
        names = arrayOf("--grammar-version"),
        help =
          """
          The grammar compatibility version to use.$NEWLINE
          ${GrammarVersion.entries.joinToString("$NEWLINE", prefix = "  ") {
            val default = if (it == GrammarVersion.latest()) " `(default)`" else ""
            "`${it.version}`: ${it.versionSpan}$default"
          }}
          """
            .trimIndent(),
      )
      .enum<GrammarVersion> { "${it.version}" }
      .default(GrammarVersion.latest())

  val overwrite: Boolean by
    option(
        names = arrayOf("-w", "--write"),
        help = "Format files in place, overwriting them. Implies `--names`",
      )
      .flag(default = false)

  val names: Boolean by
    option(
        names = arrayOf("--names"),
        help = "Write the path of files with formatting violations to stdout",
      )
      .flag(default = false)

  override fun run() {
    CliFormatterCommand(paths, grammarVersion, overwrite, names).run()
  }
}


Review comment (suggestion):
```suggestion
        help = "Write the path of files with formatting violations to stdout.",
```

Code after:
        help =
          """
          The grammar compatibility version to use.$NEWLINE
          ${GrammarVersion.entries.joinToString("$NEWLINE", prefix = "  ") {
            val default = if (it == GrammarVersion.latest()) " `(default)`" else ""
            "`${it.version}`: ${it.versionSpan}$default"
          }}
          """
            .trimIndent(),
      )
      .enum<GrammarVersion> { "${it.version}" }
      .default(GrammarVersion.latest())

  val overwrite: Boolean by
    option(
        names = arrayOf("-w", "--write"),
        help = "Format files in place, overwriting them. Implies `---diff-name-only`.",
      )
      .flag(default = false)

  val diffNameOnly: Boolean by
    option(
        names = arrayOf("--diff-name-only"),
        help = "Write the path of files with formatting violations to stdout.",
      )
      .flag(default = false)

  val silent: Boolean by
    option(
        names = arrayOf("-s", "--silent"),
        help = "Don't write to stdout or stderr. Mutually exclusive with `--diff-name-only`.",
      )
      .flag(default = false)

  override fun run() {
    CliFormatterCommand(paths, grammarVersion, overwrite, diffNameOnly, silent).run()
  }
}
Repository: apple/pkl
File: pkl-cli/src/main/kotlin/org/pkl/cli/CliFormatterCommand.kt (Kotlin)

Code before:
        val formatted = format(contents)
        if (contents != formatted) {
          status.update(VALIDATION_ERROR)
          if (names || overwrite) {
            // if `--names` or `-w` is specified, only write file names
            consoleWriter.write(path.toAbsolutePath().toString())
          } else {
            consoleWriter.write(formatted)
          }
          consoleWriter.appendLine()
          consoleWriter.flush()

          if (overwrite) {
            path.writeText(formatted, Charsets.UTF_8)
          }
        }
      } catch (pe: GenericParserError) {
        writeErr("Could not format `$path`: $pe")
        status.update(ERROR)
      } catch (e: IOException) {
        writeErr("IO error while reading `$path`: ${e.message}")
        status.update(ERROR)
      }
    }

    throw ProgramResult(status.status)
  }

  companion object {
    private const val SUCCESS = 0
    private const val VALIDATION_ERROR = 11
    private const val ERROR = 1

    private class Status(var status: Int) {
      fun update(newStatus: Int) {
        status = if (newStatus == ERROR || newStatus > status) newStatus else status
      }
    }
  }
}


Review comment (suggestion):
Within `doRun`, we should be throwing `CliException`

```suggestion
    when (status.status) {
      VALIDATION_ERROR -> throw CliException("Formatting violations were found.", status.status)
      ERROR -> throw CliException("An error occurred during formatting.", status.status)
    }
```

Code after:
            .map(ModuleSource::path)
        else -> Stream.of(ModuleSource.path(path))
      }
    }
  }

  override fun doRun() {
    val status = Status(SUCCESS)

    handleSources(status)

    when (status.status) {
      FORMATTING_VIOLATION -> {
        // using CliTestException instead of CliException because we want full control on how to
        // print errors
        throw CliTestException("", status.status)
      }
      ERROR -> {
        if (!silent) {
          writeErr("An error occurred during formatting.")
        }
        throw CliTestException("", status.status)
      }
    }
  }

  private fun handleSources(status: Status) {
    for (source in allSources()) {
      val path = if (source.uri == VmUtils.REPL_TEXT_URI) Path.of("-") else Path.of(source.uri)
      try {
        val contents =
          if (source.contents != null) {
            if (overwrite) {
              writeErr("Cannot write to stdin.")
              throw CliTestException("", ERROR)
            }
            source.contents!!
          } else {
            File(source.uri).readText()
          }

        val formatted = format(contents)
        if (contents != formatted) {
          status.update(FORMATTING_VIOLATION)
          if (diffNameOnly || overwrite) {
            // if `--diff-name-only` or `-w` is specified, only write file names
            write(path.toAbsolutePath().toString())
          }

          if (overwrite) {
            path.writeText(formatted, Charsets.UTF_8)
Repository: apple/pkl
File: pkl-cli/src/main/kotlin/org/pkl/cli/CliFormatterCommand.kt (Kotlin)

Code before:
          consoleWriter.flush()

          if (overwrite) {
            path.writeText(formatted, Charsets.UTF_8)
          }
        }
      } catch (pe: GenericParserError) {
        writeErr("Could not format `$path`: $pe")
        status.update(ERROR)
      } catch (e: IOException) {
        writeErr("IO error while reading `$path`: ${e.message}")
        status.update(ERROR)
      }
    }

    throw ProgramResult(status.status)
  }

  companion object {
    private const val SUCCESS = 0
    private const val VALIDATION_ERROR = 11
    private const val ERROR = 1

    private class Status(var status: Int) {
      fun update(newStatus: Int) {
        status = if (newStatus == ERROR || newStatus > status) newStatus else status
      }
    }
  }
}


Review comment (suggestion):
I feel that the hierarchy should be:

```
ERROR > VALIDATION_ERROR > SUCCESS
```

If an error was found, we shouldn't overwrite the status with `VALIDATION_ERROR`.

Code after:

    when (status.status) {
      FORMATTING_VIOLATION -> {
        // using CliTestException instead of CliException because we want full control on how to
        // print errors
        throw CliTestException("", status.status)
      }
      ERROR -> {
        if (!silent) {
          writeErr("An error occurred during formatting.")
        }
        throw CliTestException("", status.status)
      }
    }
  }

  private fun handleSources(status: Status) {
    for (source in allSources()) {
      val path = if (source.uri == VmUtils.REPL_TEXT_URI) Path.of("-") else Path.of(source.uri)
      try {
        val contents =
          if (source.contents != null) {
            if (overwrite) {
              writeErr("Cannot write to stdin.")
              throw CliTestException("", ERROR)
            }
            source.contents!!
          } else {
            File(source.uri).readText()
          }

        val formatted = format(contents)
        if (contents != formatted) {
          status.update(FORMATTING_VIOLATION)
          if (diffNameOnly || overwrite) {
            // if `--diff-name-only` or `-w` is specified, only write file names
            write(path.toAbsolutePath().toString())
          }

          if (overwrite) {
            path.writeText(formatted, Charsets.UTF_8)
          }
        }

        if (!diffNameOnly && !overwrite) {
          write(formatted)
        }
      } catch (pe: GenericParserError) {
        writeErr("Could not format `$path`: $pe")
        status.update(ERROR)
      } catch (e: IOException) {
Repository: apple/pkl
File: pkl-cli/src/main/kotlin/org/pkl/cli/CliFormatterCommand.kt (Kotlin)

Code before:
            consoleWriter.write(path.toAbsolutePath().toString())
          } else {
            consoleWriter.write(formatted)
          }
          consoleWriter.appendLine()
          consoleWriter.flush()

          if (overwrite) {
            path.writeText(formatted, Charsets.UTF_8)
          }
        }
      } catch (pe: GenericParserError) {
        writeErr("Could not format `$path`: $pe")
        status.update(ERROR)
      } catch (e: IOException) {
        writeErr("IO error while reading `$path`: ${e.message}")
        status.update(ERROR)
      }
    }

    throw ProgramResult(status.status)
  }

  companion object {
    private const val SUCCESS = 0
    private const val VALIDATION_ERROR = 11
    private const val ERROR = 1

    private class Status(var status: Int) {
      fun update(newStatus: Int) {
        status = if (newStatus == ERROR || newStatus > status) newStatus else status
      }
    }
  }
}


Review comment (suggestion):
[nit]

```suggestion
    private const val FORMATTING_VIOLATION = 11
```

Code after:

  override fun doRun() {
    val status = Status(SUCCESS)

    handleSources(status)

    when (status.status) {
      FORMATTING_VIOLATION -> {
        // using CliTestException instead of CliException because we want full control on how to
        // print errors
        throw CliTestException("", status.status)
      }
      ERROR -> {
        if (!silent) {
          writeErr("An error occurred during formatting.")
        }
        throw CliTestException("", status.status)
      }
    }
  }

  private fun handleSources(status: Status) {
    for (source in allSources()) {
      val path = if (source.uri == VmUtils.REPL_TEXT_URI) Path.of("-") else Path.of(source.uri)
      try {
        val contents =
          if (source.contents != null) {
            if (overwrite) {
              writeErr("Cannot write to stdin.")
              throw CliTestException("", ERROR)
            }
            source.contents!!
          } else {
            File(source.uri).readText()
          }

        val formatted = format(contents)
        if (contents != formatted) {
          status.update(FORMATTING_VIOLATION)
          if (diffNameOnly || overwrite) {
            // if `--diff-name-only` or `-w` is specified, only write file names
            write(path.toAbsolutePath().toString())
          }

          if (overwrite) {
            path.writeText(formatted, Charsets.UTF_8)
          }
        }

        if (!diffNameOnly && !overwrite) {
          write(formatted)
Repository: apple/pkl
File: pkl-core/src/main/java/org/pkl/core/util/yaml/YamlEmitter.java (Java)

Code before:
      builder.insert(pos, "? ").append('\n').append(currIndent);
    }
  }

  public final void emit(long value) {
    builder.append(value);
  }

  public final void emit(double value) {
    builder.append(
        Double.isNaN(value)
            ? ".NaN"
            : value == Double.POSITIVE_INFINITY
                ? ".Inf"
                : value == Double.NEGATIVE_INFINITY ? "-.Inf" : value);
  }

  public final void emit(boolean value) {
    builder.append(value);
  }

  public final void emitNull() {
    builder.append("null");
  }

  /** `Inf` and `NaN` are already taken care of by {@link #isReservedWord(String)}. */
  protected abstract boolean isNumber(String str, int colonIndex);

  public final String getResult() {
    return builder.toString();
  }

  protected abstract boolean isReservedWord(String str);

  protected static boolean isReserved11Word(String str) {
    if (str.length() > 5) return false;

    return switch (str) {
      case "",
          "~",
          "null",
          "Null",
          "NULL",
          ".nan",
          ".NaN",
          ".NAN",
          ".inf",
          ".Inf",
          ".INF",
          "+.inf",
          "+.Inf",

Review comment (suggestion):
We should handle empty bytes; one way to do it is `!!binary ''`.

Perhaps a better approach is to append the tag, and then call `org.pkl.core.util.yaml.YamlEmitter#emit(java.lang.String, java.lang.StringBuilder, boolean)`. That will give you a well-formed string scalar.

Code after:
    // for which 1024 characters is a safe approximation.)
    if (isKey && (builder.length() - pos > 1024 || !needsEscaping && newlineIndex != -1)) {
      builder.insert(pos, "? ").append('\n').append(currIndent);
    }
  }

  public final void emit(long value) {
    builder.append(value);
  }

  public final void emit(double value) {
    builder.append(
        Double.isNaN(value)
            ? ".NaN"
            : value == Double.POSITIVE_INFINITY
                ? ".Inf"
                : value == Double.NEGATIVE_INFINITY ? "-.Inf" : value);
  }

  public final void emit(boolean value) {
    builder.append(value);
  }

  public final void emit(byte[] value, StringBuilder currIndent, boolean isKey) {
    builder.append("!!binary ");
    emit(Base64.getEncoder().encodeToString(value), currIndent, isKey);
  }

  public final void emitNull() {
    builder.append("null");
  }

  /** `Inf` and `NaN` are already taken care of by {@link #isReservedWord(String)}. */
  protected abstract boolean isNumber(String str, int colonIndex);

  public final String getResult() {
    return builder.toString();
  }

  protected abstract boolean isReservedWord(String str);

  protected static boolean isReserved11Word(String str) {
    if (str.length() > 5) return false;

    return switch (str) {
      case "",
          "~",
          "null",
          "Null",
          "NULL",
          ".nan",
Repository: apple/pkl
File: pkl-formatter/src/main/kotlin/org/pkl/formatter/Builder.kt (Kotlin)

Code before:
      }

      return NaturalOrderComparator(ignoreCase = true).compare(import1, import2)
    }
  }

  private fun Node.firstProperChild(): Node? {
    for (child in children) {
      if (child.isProper()) return child
    }
    return null
  }

  private fun List<Node>.lastProperNode(): Node? {
    for (i in lastIndex downTo 0) {
      if (this[i].isProper()) return this[i]
    }
    return null
  }

  // returns true if this node is not an affix or terminal
  private fun Node.isProper(): Boolean = !type.isAffix && type != NodeType.TERMINAL

  private fun Node.isMultiline(): Boolean = span.lineBegin < span.lineEnd

  private fun List<Node>.isMultiline(): Boolean = first().span.lineBegin < last().span.lineEnd

  private inline fun <T> List<T>.splitOn(pred: (T) -> Boolean): Pair<List<T>, List<T>> {
    val index = indexOfFirst { pred(it) }
    return if (index == -1) {
      Pair(this, emptyList())
    } else {
      Pair(take(index), drop(index))
    }
  }

  class PeekableIterator<T>(private val iterator: Iterator<T>) : Iterator<T> {
    private var peek: T? = null

    private var hasPeek = false

    override fun next(): T {
      return if (hasPeek) {
        hasPeek = false
        peek!!
      } else {
        iterator.next()
      }
    }

    override fun hasNext(): Boolean {

Review comment (suggestion):
```suggestion
  private fun List<Node>.isMultiline(): Boolean =
    if (isEmpty()) false else first().span.lineBegin < last().span.lineEnd
```
Better safe than sorry.

Code after:
      }

      return NaturalOrderComparator(ignoreCase = true).compare(import1, import2)
    }
  }

  private fun Node.firstProperChild(): Node? {
    for (child in children) {
      if (child.isProper()) return child
    }
    return null
  }

  private fun List<Node>.lastProperNode(): Node? {
    for (i in lastIndex downTo 0) {
      if (this[i].isProper()) return this[i]
    }
    return null
  }

  // returns true if this node is not an affix or terminal
  private fun Node.isProper(): Boolean = !type.isAffix && type != NodeType.TERMINAL

  private fun Node.isMultiline(): Boolean = span.lineBegin < span.lineEnd

  private fun List<Node>.isMultiline(): Boolean =
    if (isEmpty()) false else first().span.lineBegin < last().span.lineEnd

  private inline fun <T> List<T>.splitOn(pred: (T) -> Boolean): Pair<List<T>, List<T>> {
    val index = indexOfFirst { pred(it) }
    return if (index == -1) {
      Pair(this, emptyList())
    } else {
      Pair(take(index), drop(index))
    }
  }

  class PeekableIterator<T>(private val iterator: Iterator<T>) : Iterator<T> {
    private var peek: T? = null

    private var hasPeek = false

    override fun next(): T {
      return if (hasPeek) {
        hasPeek = false
        peek!!
      } else {
        iterator.next()
      }
    }