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

Add explicit execution context configuration #695

Merged
merged 8 commits into from
Mar 7, 2022
Merged

Add explicit execution context configuration #695

merged 8 commits into from
Mar 7, 2022

Conversation

retriku
Copy link
Contributor

@retriku retriku commented Feb 18, 2022

Adds configurable execution context while creating RabbitClient. Closes issue #694

@jbwheatley
Copy link
Contributor

Yeah, think this looks like the most sensible way to add a configurable ExecutionContext 👍

You will unfortunately have do some work to get mima to pass - I think you can do this by new functions with the added argument and deprecating/making package private the old one. i.e.

@deprecated("use other apply method with configurable execution context", "4.1.1")
private[interpreter] def apply[F[_]: Async](
      config: Fs2RabbitConfig,
      dispatcher: Dispatcher[F],
      sslContext: Option[SSLContext] = None,
      saslConfig: SaslConfig = DefaultSaslConfig.PLAIN,
      metricsCollector: Option[MetricsCollector] = None,
      threadFactory: Option[F[ThreadFactory]] = None
  ): F[RabbitClient[F]] = ???

def apply[F[_]: Async](
      config: Fs2RabbitConfig,
      dispatcher: Dispatcher[F],
      sslContext: Option[SSLContext] = None,
      saslConfig: SaslConfig = DefaultSaslConfig.PLAIN,
      metricsCollector: Option[MetricsCollector] = None,
      threadFactory: Option[F[ThreadFactory]] = None,
      executionContext: Option[F[ExecutionContext]] = None
  ): F[RabbitClient[F]] = ???

Add execution context config to RabbitClient.resource as well
@retriku
Copy link
Contributor Author

retriku commented Mar 2, 2022

Hi @jbwheatley! I do not know how to solve these migration issues. Maybe we need to increment major version?

@jbwheatley
Copy link
Contributor

ah I think you're right, can't get it to work myself either. Default arguments are messy to work with 😿
I think in the future I'd consider replacing the client apply methods with a builder pattern.

Thanks for doing the legwork on this PR, if you have the time it would be great if we could go back to just adding the argument to the methods as you had it originally, and then we'll go for a 5.0.0 release

@retriku
Copy link
Contributor Author

retriku commented Mar 4, 2022

I introduced builder for RabbitClient. Please, review

@jbwheatley
Copy link
Contributor

Awesome, the builder stuff is great 👍

Can you keep the existing apply and resource methods on RabbitClient, and deprecate them - pointing to the new builder class? That makes it easier for people to make the change when they upgrade.

@retriku
Copy link
Contributor Author

retriku commented Mar 7, 2022

@jbwheatley done!

@jbwheatley jbwheatley merged commit 2ae3814 into profunktor:master Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants