Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create ChuckerInterceptor from builder #462

Closed
1 of 2 tasks
MiSikora opened this issue Sep 30, 2020 · 3 comments · Fixed by #483
Closed
1 of 2 tasks

Create ChuckerInterceptor from builder #462

MiSikora opened this issue Sep 30, 2020 · 3 comments · Fixed by #483
Labels
enhancement New feature or improvement to the library

Comments

@MiSikora
Copy link
Contributor

MiSikora commented Sep 30, 2020

⚠️ Is your feature request related to a problem? Please describe

Having multi-arg constructor is nice from Kotlin perspective but not that great from Java and binary compatibility. I'd like to introduce in 4.x builder pattern.

💡 Describe the solution you'd like

It would look more or less like this. I omitted most input parameters for brevity.

class ChuckerInterceptor internal constructor(builder: Builder) : Interceptor {
  private val context = builder.context
  private val collector = builder.collector
  private val maxContentLength = builder.maxContentLength

  public constructor(context: Context) : this(Builder(context))

  class Builder(internal val context: Context) {
    internal var collector = ChuckerCollector(context)
    internal var maxContentLength = 250_000L

    fun collector(collector: ChuckerCollector) = apply { this.collector = collector }
    
    fun maxContentLength(maxContentLength: Long) = apply { this.maxContentLength = maxContentLength }

    fun build() = ChuckerInterceptor(this) 
  }
}

This approach is nice because users that don't need customization can create an interceptor with ChuckerInterceptor(context) and others can customize it nicely from both Kotlin and Java worlds.

📊 Describe alternatives you've considered

I know there was a proposal for the DSL approach in #141. It would still be viable. We could hide the builder from Kotlin and hide DSL from Java. However, I don't see much value in DSL here and I feel like adding DSL would only be "fancy" and not useful.

📄 Additional context

We could make migration easier by deprecating the current constructor in 3.3.1 and by adding a builder to which we encourage migration.

One downside of this is that we would encourage users to migrate away from the constructor even if they use one that accepts only Context, which would be available in 4.x. I don't think there is any way around it, unfortunately. This could be explained more in the deprecation message.

🙋 Do you want to develop this feature yourself?

  • Yes
  • No
@MiSikora MiSikora changed the title Create Chucker with from builder Create ChuckerInterceptor from builder Sep 30, 2020
@MiSikora MiSikora added discussion enhancement New feature or improvement to the library labels Sep 30, 2020
@MiSikora
Copy link
Contributor Author

MiSikora commented Oct 1, 2020

One downside of this, is that we would encourage users to migrate away from the constructor even if they use one that accepts only Context, which would be available in 4.x. I don't think there is any way around it, unfortunately. This could be explained more in the deprecation message.

Actually, I might be wrong about this. It probably can handled with removing @JvmOverloads from our current constructor.

@vbuberen
Copy link
Collaborator

vbuberen commented Oct 3, 2020

I love the idea of having a good old builder pattern to configure Chucker, so totally in for this.

As to DSL - I am also not a big fan of it. For example, here is an article with some notes on Coil library maintainer experience with DSL and why Coil moved to builders: https://medium.com/@colinwhite/prefer-builders-over-dsls-3dc7fc375d47

@vbuberen
Copy link
Collaborator

vbuberen commented Oct 3, 2020

Talking about migration - well, in 4.x we will have to introduce some breaking changes anyway, like those from database redesign.

@ghost ghost added the Pending PR The resolution for the issue is in PR label Oct 23, 2020
@ghost ghost removed the Pending PR The resolution for the issue is in PR label Nov 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement to the library
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants