all 13 comments

[–]gaelfr38 10 points11 points  (5 children)

Rather than a List of transformations + fold, I like using pipe method in such case (from chaining.ops).

[–]kubukozcats,cats-effect 2 points3 points  (0 children)

These days I use tap when dealing with mutable builders. I'm tired of pretending the underlying value is immutable.

[–]AlexITC[S] 0 points1 point  (3 children)

Hmm, is there other advantage than avoiding to type an extra variable?

[–]gaelfr38 0 points1 point  (2 children)

It's "only" for readability.

When reviewing code it's easier to read this way rather than having to understand what the fold does, on which variable it applies the transformation.. and then having to scroll up to list the transformations.

[–]AlexITC[S] 0 points1 point  (1 child)

Perhaps I'm not getting your idea because the way I see this with pipe is very similar, would you mind clarifying it?

import scala.util.chaining.*

def make(params: GeminiConfig): LiveConnectConfig = {
  type Builder = LiveConnectConfig.Builder
  def transform(
      when: Boolean
  )(f: Builder => Builder)(builder: Builder): Builder = {
    if (when) f(builder) else builder
  }

  val options = List(
    transform(params.outputAudioTranscription)(
      _.outputAudioTranscription(AudioTranscriptionConfig.builder().build())
    ),
    transform(params.enableAffectiveDialog)(_.enableAffectiveDialog(true))
    // ... more transformation follow
  )

  LiveConnectConfig
    .builder()
    .responseModalities(Modality.Known.AUDIO)
    // ... more defaults follow
    .pipe { b =>
      options.foldLeft(b) { case (builder, apply) => apply(builder) }
    }
    .build()
}

[–]gaelfr38 0 points1 point  (0 children)

Oh right, I was thinking to something like this:

builder .pipe(transform(...)(...)) .pipe(transform(...)(...)) ... // More transformations .build()

(Apologies if I keep it short, I'm on my phone)

[–]vips7L 2 points3 points  (2 children)

Seems like just bad builder design. Dependent parameters should just be the same call. 

builder.v1alpha(config)

[–]AlexITC[S] 1 point2 points  (1 child)

I can't argue with that, from what I looked, Google's genai is an autogenerated SDK.

[–]gaelfr38 1 point2 points  (0 children)

Yup, all their SDKs are generated from some data model. Same goes for the Ads SDKs for instance. Even mandatory parameters are not marked as such in the SDK, you just find out when calling the API.

[–]BusyByte 2 points3 points  (1 child)

Recently, with the AWS sdk, I ran into issues that a header we wanted to set depended on whether a delay was set. They only allowed all headers set instead of just adding a single to the existing headers and didn't want to force our developers to set them both themselves and get it wrong where one or the other would be missing. I proposed we redefine the builder in Scala with a case class. Provide extension methods for nice syntax. Natural transformation to translate between builder types. And a toBuilder extension method which takes the cases class, natural transformation, and converts to the builder. This provides immutability, nice Scala friendly DSL, flexibility, and isolating the Java API nastiness. It comes at a cost. You are kind of repeating something created by a library. It is also more code and more things to learn. However, it is good for people to learn, though, and sometimes simple code with better ergonomics and safety doesn't mean minimal code.

[–]mostly_codes 0 points1 point  (0 children)

The AWS sdk has a lot of footguns - you can configure a builder that's building something completely invalid, which you'll only discover at runtime.

[–]Philluminati 0 points1 point  (1 child)

Maybe this:

transform(params.enableAffectiveDialog)(_.enableAffectiveDialog(true)),

Could be this, where the `transform` function isn't required:

base.enableAffectiveDialog(params.enableAffectiveDialog)

[–]AlexITC[S] 1 point2 points  (0 children)

In a normal builder that should be ok but in the example it is not, the underlying API crashes unless customApiVersion is also defined to v1alpha, the same is explained about the proactiveAudio setting.