ronantakizawa/codereview-bench huggingface dataset:
Repository: expo/expo
File: packages/expo-modules-core/ios/Core/Records/Formatter.swift (Swift)

Code before:
/**
 A marker object that allows us to skip calculating a property.
 */
enum Skip { case instance }

class PropertySelector<RecordType: Record> {
  class ActionBuilder<InputType> {
    typealias NextBuilder<T> = PropertySelector.ActionBuilder<T>

    private let propertySelector: PropertySelector<RecordType>

    init (selector: PropertySelector<RecordType>) {
      propertySelector = selector
    }

    @discardableResult func map<ResultType>(
      _ mapper: @escaping (RecordType, InputType) -> ResultType
    ) -> NextBuilder<ResultType> {
      return chainAction { record, value in mapper(record, value as! InputType ) }
    }

    @discardableResult func map<ResultType>(
      _ mapper: @escaping ( InputType) -> ResultType
    ) -> NextBuilder<ResultType> {
      return chainAction { _, value in mapper(value as! InputType ) }
    }

    @discardableResult func skip() -> NextBuilder<InputType> {
      return chainSkipAction(shouldSkip: { _, _ in true })
    }

    @discardableResult func skip(
      _ valueSelector: @escaping (RecordType, InputType) -> Bool
    ) -> NextBuilder<InputType> {
      return chainSkipAction(shouldSkip: valueSelector)
    }

    @discardableResult func skip(
      _ valueSelector: @escaping (InputType) -> Bool
    ) -> NextBuilder<InputType> {
      return chainSkipAction { _, value in valueSelector(value) }

Review comment (suggestion):
```suggestion
    @discardableResult
    func map<ResultType>(
```

Code after:
/**
 A marker object that allows us to skip calculating a property.
 */
enum Skip {
  case instance
}

public class PropertySelector<RecordType: Record> {
  public class ActionBuilder<InputType> {
    public typealias NextBuilder<T> = PropertySelector.ActionBuilder<T>

    private let propertySelector: PropertySelector<RecordType>

    internal init(selector: PropertySelector<RecordType>) {
      propertySelector = selector
    }

    @discardableResult
    public func map<ResultType>(
      _ mapper: @escaping (RecordType, InputType) -> ResultType
    ) -> NextBuilder<ResultType> {
      return chainAction { record, value in mapper(record, value as! InputType ) }
    }

    @discardableResult
    public func map<ResultType>(
      _ mapper: @escaping ( InputType) -> ResultType
    ) -> NextBuilder<ResultType> {
      return chainAction { _, value in mapper(value as! InputType ) }
    }

    @discardableResult
    public func skip() -> NextBuilder<InputType> {
      return chainSkipAction(shouldSkip: { _, _ in true })
    }

    @discardableResult
    public func skip(
      _ valueSelector: @escaping (RecordType, InputType) -> Bool
    ) -> NextBuilder<InputType> {
      return chainSkipAction(shouldSkip: valueSelector)
Repository: expo/expo
File: packages/expo-modules-core/ios/Core/Records/Formatter.swift (Swift)

Code before:
/**
 A marker object that allows us to skip calculating a property.
 */
enum Skip { case instance }

class PropertySelector<RecordType: Record> {
  class ActionBuilder<InputType> {
    typealias NextBuilder<T> = PropertySelector.ActionBuilder<T>

    private let propertySelector: PropertySelector<RecordType>

    init (selector: PropertySelector<RecordType>) {
      propertySelector = selector
    }

    @discardableResult func map<ResultType>(
      _ mapper: @escaping (RecordType, InputType) -> ResultType
    ) -> NextBuilder<ResultType> {
      return chainAction { record, value in mapper(record, value as! InputType ) }
    }

    @discardableResult func map<ResultType>(
      _ mapper: @escaping ( InputType) -> ResultType
    ) -> NextBuilder<ResultType> {
      return chainAction { _, value in mapper(value as! InputType ) }
    }

    @discardableResult func skip() -> NextBuilder<InputType> {
      return chainSkipAction(shouldSkip: { _, _ in true })
    }

    @discardableResult func skip(
      _ valueSelector: @escaping (RecordType, InputType) -> Bool
    ) -> NextBuilder<InputType> {
      return chainSkipAction(shouldSkip: valueSelector)
    }

    @discardableResult func skip(
      _ valueSelector: @escaping (InputType) -> Bool
    ) -> NextBuilder<InputType> {
      return chainSkipAction { _, value in valueSelector(value) }
    }

    private func chainAction<NextInputType>(
      nextAction: @escaping (RecordType, Any?) -> Any?
    ) -> NextBuilder<NextInputType> {
      let previouseAction = propertySelector.action

Review comment (suggestion):
```suggestion
    @discardableResult
    func map<ResultType>(
```

Code after:
/**
 A marker object that allows us to skip calculating a property.
 */
enum Skip {
  case instance
}

public class PropertySelector<RecordType: Record> {
  public class ActionBuilder<InputType> {
    public typealias NextBuilder<T> = PropertySelector.ActionBuilder<T>

    private let propertySelector: PropertySelector<RecordType>

    internal init(selector: PropertySelector<RecordType>) {
      propertySelector = selector
    }

    @discardableResult
    public func map<ResultType>(
      _ mapper: @escaping (RecordType, InputType) -> ResultType
    ) -> NextBuilder<ResultType> {
      return chainAction { record, value in mapper(record, value as! InputType ) }
    }

    @discardableResult
    public func map<ResultType>(
      _ mapper: @escaping ( InputType) -> ResultType
    ) -> NextBuilder<ResultType> {
      return chainAction { _, value in mapper(value as! InputType ) }
    }

    @discardableResult
    public func skip() -> NextBuilder<InputType> {
      return chainSkipAction(shouldSkip: { _, _ in true })
    }

    @discardableResult
    public func skip(
      _ valueSelector: @escaping (RecordType, InputType) -> Bool
    ) -> NextBuilder<InputType> {
      return chainSkipAction(shouldSkip: valueSelector)
    }

    @discardableResult
    public func skip(
      _ valueSelector: @escaping (InputType) -> Bool
    ) -> NextBuilder<InputType> {
Repository: expo/expo
File: packages/expo-modules-core/ios/Core/Records/Formatter.swift (Swift)

Code before:
        if previousValue is Skip {
          return Skip.instance
        }

        return nextAction(record, previousValue)
      }

      return PropertySelector.ActionBuilder(selector: propertySelector)
    }

    private func chainSkipAction(
      shouldSkip: @escaping (RecordType, InputType) -> Bool
    ) -> NextBuilder<InputType> {
      return chainAction { record, value in
        let typedValue = value as! InputType
        if shouldSkip(record, typedValue) {
          return Skip.instance
        }

        return typedValue
      }
    }
  }

  let name: String
  internal var action: (RecordType, Any?) -> Any?

  init (name: String) {
    self.name = name
    self.action = { _, value in value }
  }
}

class Formatter<RecordType: Record> {
  internal var selectors: [PropertySelector<RecordType>] = []

  func property<PropertyType>(
    _ name: String,
    keyPath: KeyPath<RecordType, PropertyType>
  ) -> PropertySelector<RecordType>.ActionBuilder<PropertyType> {
    let selector = PropertySelector<RecordType>(name: name)
    selectors.append(selector)
    return PropertySelector.ActionBuilder(selector: selector)
  }
}


Review comment (suggestion):
```suggestion
  internal let name: String
  internal var action: (RecordType, Any?) -> Any?
```

Code after:

    private func chainAction<NextInputType>(
      nextAction: @escaping (RecordType, Any?) -> Any?
    ) -> NextBuilder<NextInputType> {
      let previousAction = propertySelector.action
      propertySelector.action = { record, value in
        let previousValue = previousAction(record, value)
        if previousValue is Skip {
          return Skip.instance
        }

        return nextAction(record, previousValue)
      }

      return PropertySelector.ActionBuilder(selector: propertySelector)
    }

    private func chainSkipAction(
      shouldSkip: @escaping (RecordType, InputType) -> Bool
    ) -> NextBuilder<InputType> {
      return chainAction { record, value in
        let typedValue = value as! InputType
        if shouldSkip(record, typedValue) {
          return Skip.instance
        }

        return typedValue
      }
    }
  }

  internal let name: String
  internal var action: (RecordType, Any?) -> Any?

  internal init(name: String) {
    self.name = name
    self.action = { _, value in value }
  }
}

public class Formatter<RecordType: Record> {
  internal var selectors: [PropertySelector<RecordType>] = []

  public func property<PropertyType>(
    _ name: String,
    keyPath: KeyPath<RecordType, PropertyType>
  ) -> PropertySelector<RecordType>.ActionBuilder<PropertyType> {
    let selector = PropertySelector<RecordType>(name: name)
    selectors.append(selector)
    return PropertySelector.ActionBuilder(selector: selector)
  }
Repository: expo/expo
File: packages/expo-modules-core/ios/Core/Records/FormattedRecord.swift (Swift)

Code before:

        if let action = action {
          let formattedValue = action(record, field.get())
          if !(formattedValue is Skip) {
            result[key] = Conversions.convertFunctionResult(formattedValue, appContext: appContext)
          }
        } else {
          result[key] = Conversions.convertFunctionResult(field.get(), appContext: appContext)
        }
      }
    }
  }

  static func convertResult(_ result: Any, appContext: AppContext) throws -> Any {
    if let value = result as? FormattedRecord {
      return value.toDictionary(appContext: appContext)
    }
    return result
  }

  static func convert(from value: Any?, appContext: AppContext) throws -> FormattedRecord<RecordType> {
    throw FormattedRecordCannotBeUsedAsParameterException()
  }
}

func format<RecordType: Record>(
  record: RecordType,
  formatter: Formatter<RecordType>
) -> FormattedRecord<RecordType> {
  return FormattedRecord(record: record, formatter: formatter)
}


Review comment (suggestion):
Does it have to be a global function? What about `formatter.format(record:)` instead? Or even better, `record.format { formatter in ... }`.

I guess its intention was to be public, so you should add `public` – in Swift `internal` access is the default in most cases.

Code after:
/**
 Class that binds a formatter with a record.
 It can be converted to JS, but it can't be converted from a JS value.
 */
public struct FormattedRecord<RecordType: Record>: Convertible {
  internal final class FormattedRecordCannotBeUsedAsParameterException: Exception, @unchecked Sendable {
    override var reason: String {
      "FormattedRecord cannot be used as a parameter"
    }
  }

  internal let record: RecordType
  internal let formatter: Formatter<RecordType>

  func toDictionary(appContext: AppContext? = nil) -> Record.Dict {
    return fieldsOf(record).reduce(into: Record.Dict()) { result, field in
      if let key = field.key {
        if let action = formatter.selectors.first(where: { $0.name == key })?.action {
          let formattedValue = action(record, field.get())
          if !(formattedValue is Skip) {
            result[key] = Conversions.convertFunctionResult(formattedValue, appContext: appContext)
          }
        } else {
          result[key] = Conversions.convertFunctionResult(field.get(), appContext: appContext)
        }
      }
    }
  }

  public static func convertResult(_ result: Any, appContext: AppContext) throws -> Any {
    if let value = result as? FormattedRecord {
      return value.toDictionary(appContext: appContext)
    }
    return result
  }

  public static func convert(from value: Any?, appContext: AppContext) throws -> FormattedRecord<RecordType> {
    throw FormattedRecordCannotBeUsedAsParameterException()
  }
}
Repository: expo/expo
File: packages/expo-modules-core/FormatterTests.swift (Swift)

Code before:
// Copyright 2022-present 650 Industries. All rights reserved.
import Testing
import ExpoModulesTestCore

@testable import ExpoModulesCore

@Suite
struct FormatterTests {
  @Suite("E2E")
  struct E2E {
    let appContext: AppContext

    init() {
      self.appContext = AppContext.create()
      self.appContext.moduleRegistry.register(moduleType: FormatterTestModule.self, name: nil)
    }

    @Test("can be used in sync functions")
    func syncFunctions() throws {
      let r1 = try appContext.runtime
        .eval("expo.modules.FormatterModule.f1()")
        .asObject()

      let r2 = try appContext.runtime
        .eval("expo.modules.FormatterModule.f2()")
        .asObject()

      #expect(r1.getProperty("a").isUndefined() == true)
      #expect(r1.getProperty("b").getString() == "b")
      #expect(r1.getProperty("c").getString() == "c")

      #expect(r2.getProperty("a").getString() == "d")
      #expect(r2.getProperty("b").getString() == "default")
      #expect(r2.getProperty("c").isUndefined() == true)
    }

    @Test("can be used in async functions")
    func asyncFunctions() async throws {
      try appContext.runtime.eval(
        "expo.modules.FormatterModule.f1Async().then((result) => { globalThis.result = result; })"
      )
      try await expectEventually {
        try appContext.runtime.eval("!!globalThis.result").asBool() == true
      }

Review comment (suggestion):
You can write it like in Kotlin 😉
```suggestion
    @Test
    func `can be used in sync functions`() throws {
```

Code after:
// Copyright 2022-present 650 Industries. All rights reserved.
import Testing
import ExpoModulesTestCore

@testable import ExpoModulesCore

@Suite
struct FormatterTests {
  @Suite("E2E")
  struct E2E {
    let appContext: AppContext

    init() {
      self.appContext = AppContext.create()
      self.appContext.moduleRegistry.register(moduleType: FormatterTestModule.self, name: nil)
    }

    @Test
    func `can be used in sync functions`() throws {
      let r1 = try appContext.runtime
        .eval("expo.modules.FormatterModule.f1()")
        .asObject()

      let r2 = try appContext.runtime
        .eval("expo.modules.FormatterModule.f2()")
        .asObject()

      #expect(r1.getProperty("a").isUndefined() == true)
      #expect(r1.getProperty("b").getString() == "b")
      #expect(r1.getProperty("c").getString() == "c")

      #expect(r2.getProperty("a").getString() == "d")
      #expect(r2.getProperty("b").getString() == "default")
      #expect(r2.getProperty("c").isUndefined() == true)
    }

    @Test
    func `can be used in async functions`() async throws {
      try appContext.runtime.eval(
        "expo.modules.FormatterModule.f1Async().then((result) => { globalThis.result = result; })"
      )
      try await expectEventually {
        try appContext.runtime.eval("!!globalThis.result").asBool() == true
      }
Repository: expo/expo
File: apps/expo-go/android/expoview/src/main/java/host/exp/exponent/home/HomeAppViewModel.kt (Kotlin)

Code before:
  fun selectAccount(accountId: String?) {
    selectedAccountId.value = accountId
  }

  fun scanQR(
    context: Context,
    onSuccess: (String) -> Unit,
    onError: (String) -> Unit = {}
  ) {
    val options = GmsBarcodeScannerOptions
      .Builder()
      .setBarcodeFormats(Barcode.FORMAT_QR_CODE)
      .build()

    val scanner = GmsBarcodeScanning.getClient(context, options)

    scanner.startScan()
      .addOnSuccessListener { barcode ->
        val url = barcode.rawValue ?: run {
          onError("No QR code data found")
          return@addOnSuccessListener
        }
        onSuccess(url)
      }
      .addOnCanceledListener {
        onError("QR code scan canceled")
      }
      .addOnFailureListener { exception ->
        onError("QR code scan failed: ${exception.message ?: "Unknown error"}")
      }
  }
}

class RefreshableFlow<T>(
  val dataFlow: StateFlow<T>,
  val loadingFlow: StateFlow<Boolean>,
  val refresh: () -> Unit
)

@OptIn(ExperimentalCoroutinesApi::class)
fun <T> refreshableFlow(
  scope: CoroutineScope,
  fetcher: () -> Flow<T>,
  initialValue: T
): RefreshableFlow<T> {
  val refreshTrigger = MutableSharedFlow<Unit>(replay = 1)
  val loadingState = MutableStateFlow(false)

  refreshTrigger.tryEmit(Unit)

  val stateFlow = refreshTrigger

Review comment (suggestion):
```suggestion
        onError("QR code scan cancelled")
```

Code after:
  fun selectAccount(accountId: String?) {
    selectedAccountId.value = accountId
  }

  fun scanQR(
    context: Context,
    onSuccess: (String) -> Unit,
    onError: (String) -> Unit = {}
  ) {
    val options = GmsBarcodeScannerOptions
      .Builder()
      .setBarcodeFormats(Barcode.FORMAT_QR_CODE)
      .build()

    val scanner = GmsBarcodeScanning.getClient(context, options)

    scanner.startScan()
      .addOnSuccessListener { barcode ->
        val url = barcode.rawValue ?: run {
          onError("No QR code data found")
          return@addOnSuccessListener
        }
        onSuccess(url)
      }
      .addOnCanceledListener {
        onError("QR code scan cancelled")
      }
      .addOnFailureListener { exception ->
        onError("QR code scan failed: ${exception.message ?: "Unknown error"}")
      }
  }
}

class RefreshableFlow<T>(
  val dataFlow: StateFlow<T>,
  val loadingFlow: StateFlow<Boolean>,
  val refresh: () -> Unit
)

@OptIn(ExperimentalCoroutinesApi::class)
fun <T> refreshableFlow(
  scope: CoroutineScope,
  fetcher: () -> Flow<T>,
  initialValue: T
): RefreshableFlow<T> {
  val refreshTrigger = MutableSharedFlow<Unit>(replay = 1)
  val loadingState = MutableStateFlow(false)

  refreshTrigger.tryEmit(Unit)

  val stateFlow = refreshTrigger
Repository: expo/expo
File: packages/expo-dev-launcher/ios/Modules/DevLauncherModule.swift (Swift)

Code before:
// Copyright 2015-present 650 Industries. All rights reserved.

import ExpoModulesCore

public class DevLauncherModule: Module {
  public func definition() -> ModuleDefinition {
    Name("ExpoDevLauncher")

    AsyncFunction("loadApp") { (urlString: String, projectUrlString: String?, promise: Promise) in
      guard let url = URL(string: urlString) else {
        promise.reject(DevLauncherInvalidURLException())
        return
      }

      if let projectUrlString, URL(string: projectUrlString) == nil {
        promise.reject(DevLauncherInvalidProjectURLException())
        return
      }

      let projectUrl = projectUrlString.flatMap { URL(string: $0) }

      EXDevLauncherController.sharedInstance().loadApp(
        url,
        withProjectUrl: projectUrl,
        onSuccess: {
          promise.resolve(true)
        },
        onError: { error in
          promise.reject(DevLauncherLoadAppException(error.localizedDescription).causedBy(error))
        }
      )
    }
  }
}

Review comment (suggestion):
```suggestion
    AsyncFunction("loadApp") { (urlString: URL, projectUrlString: URL?, promise: Promise) in
```
I believe that the API can handle string to URL conversion 🤔 It should also work on Android. 

Code after:
// Copyright 2015-present 650 Industries. All rights reserved.

import ExpoModulesCore

public class DevLauncherModule: Module {
  public func definition() -> ModuleDefinition {
    Name("ExpoDevLauncher")

    AsyncFunction("loadApp") { (url: URL, promise: Promise) in
      if url.scheme == nil {
        throw DevLauncherInvalidURLException()
      }

      EXDevLauncherController.sharedInstance().loadApp(
        url,
        withProjectUrl: nil,
        onSuccess: {
          promise.resolve(true)
        },
        onError: { error in
          promise.reject(DevLauncherLoadAppException(error.localizedDescription).causedBy(error))
        }
      )
    }
  }
}
Repository: expo/expo
File: packages/expo-dev-launcher/ios/Modules/DevLauncherModule.swift (Swift)

Code before:
// Copyright 2015-present 650 Industries. All rights reserved.

import ExpoModulesCore

public class DevLauncherModule: Module {
  public func definition() -> ModuleDefinition {
    Name("ExpoDevLauncher")

    AsyncFunction("loadApp") { (url: URL, projectUrl: URL?, promise: Promise) in
      if url.scheme == nil {
        promise.reject(DevLauncherInvalidURLException())
        return
      }

      if let projectUrl, projectUrl.scheme == nil {
        promise.reject(DevLauncherInvalidProjectURLException())
        return
      }

      EXDevLauncherController.sharedInstance().loadApp(
        url,
        withProjectUrl: projectUrl,
        onSuccess: {
          promise.resolve(true)
        },
        onError: { error in
          promise.reject(DevLauncherLoadAppException(error.localizedDescription).causedBy(error))
        }
      )
    }
  }
}


Review comment (suggestion):
We can just throw it without the promise. It will automatically reject. This will help me in the future, as I want to remove promises from function arguments and make `AsyncFunction` use `ConcurrentFunctionDefinition` by default (Swift concurrency rather than pseudo-promise-based concurrency).
```suggestion
        throw DevLauncherInvalidURLException()
```

Code after:
// Copyright 2015-present 650 Industries. All rights reserved.

import ExpoModulesCore

public class DevLauncherModule: Module {
  public func definition() -> ModuleDefinition {
    Name("ExpoDevLauncher")

    AsyncFunction("loadApp") { (url: URL, promise: Promise) in
      if url.scheme == nil {
        throw DevLauncherInvalidURLException()
      }

      EXDevLauncherController.sharedInstance().loadApp(
        url,
        withProjectUrl: nil,
        onSuccess: {
          promise.resolve(true)
        },
        onError: { error in
          promise.reject(DevLauncherLoadAppException(error.localizedDescription).causedBy(error))
        }
      )
    }
  }
}
Repository: expo/expo
File: packages/expo-dev-launcher/ios/Modules/DevLauncherModule.swift (Swift)

Code before:
// Copyright 2015-present 650 Industries. All rights reserved.

import ExpoModulesCore

public class DevLauncherModule: Module {
  public func definition() -> ModuleDefinition {
    Name("ExpoDevLauncher")

    AsyncFunction("loadApp") { (url: URL, projectUrl: URL?, promise: Promise) in
      if url.scheme == nil {
        promise.reject(DevLauncherInvalidURLException())
        return
      }

      if let projectUrl, projectUrl.scheme == nil {
        promise.reject(DevLauncherInvalidProjectURLException())
        return
      }

      EXDevLauncherController.sharedInstance().loadApp(
        url,
        withProjectUrl: projectUrl,
        onSuccess: {
          promise.resolve(true)
        },
        onError: { error in
          promise.reject(DevLauncherLoadAppException(error.localizedDescription).causedBy(error))
        }
      )
    }
  }
}


Review comment (suggestion):
```suggestion
        throw DevLauncherInvalidProjectURLException()
```

Code after:
// Copyright 2015-present 650 Industries. All rights reserved.

import ExpoModulesCore

public class DevLauncherModule: Module {
  public func definition() -> ModuleDefinition {
    Name("ExpoDevLauncher")

    AsyncFunction("loadApp") { (url: URL, promise: Promise) in
      if url.scheme == nil {
        throw DevLauncherInvalidURLException()
      }

      EXDevLauncherController.sharedInstance().loadApp(
        url,
        withProjectUrl: nil,
        onSuccess: {
          promise.resolve(true)
        },
        onError: { error in
          promise.reject(DevLauncherLoadAppException(error.localizedDescription).causedBy(error))
        }
      )
    }
  }
}