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

Consumer Tag #208

Merged
merged 6 commits into from
Nov 26, 2023
Merged

Consumer Tag #208

merged 6 commits into from
Nov 26, 2023

Conversation

rlavolee
Copy link
Contributor

adds the option to define a custom consumer tag or fallback to random tag

modules/client/src/main/scala/MessagingAPI.scala Outdated Show resolved Hide resolved
modules/protocol/src/main/scala/Domains.scala Outdated Show resolved Hide resolved
@rlavolee
Copy link
Contributor Author

Hi @hnaderi !

I'm trying to get the PR done but I'm struggling with Mima.
Mima is complaining that consumerRaw is not there anymore because I added consumerTag: Option[ConsumerTag] = None in the signature.

method consumeRaw(java.lang.String,Boolean,Boolean,Boolean,scala.collection.immutable.Map)fs2.Stream in class lepus.client.Channel#ConsumingImpl does not have a correspondent in current version

I thought to add the old consumerRaw back as shown below and call the new one in its body.

  def consumeRaw(
      queue: QueueName,
      noLocal: NoLocal = false,
      noAck: NoAck = true,
      exclusive: Boolean = false,
      arguments: FieldTable = FieldTable.empty,
      consumerTag: Option[ConsumerTag] = None
  ): Stream[F, DeliveredMessageRaw]

  final def consumeRaw(
      queue: QueueName,
      noLocal: NoLocal = false,
      noAck: NoAck = true,
      exclusive: Boolean = false,
      arguments: FieldTable = FieldTable.empty
  ): Stream[F, DeliveredMessageRaw] =
    consumeRaw(queue, noLocal, noAck, exclusive, arguments, None)

However the compiler is complaining about overloaded methods because of default arguments.

two or more overloaded variants of method consumeRaw have default arguments

What's the best way to deal with this situation ? Should I remove default args, break binary comp or introduce a new method name ? 🤔

@hnaderi
Copy link
Owner

hnaderi commented Nov 26, 2023

@rlavolee

What's the best way to deal with this situation ? Should I remove default args, break binary comp or introduce a new method name ? 🤔

One way would be to add new methods instead of changing the old ones, but it's not worth it and I think we can break the bin-compat here, as the changes are source compatible.

I just bumped up the minor version and pushed it on the PR, so it should be fixed.

@hnaderi
Copy link
Owner

hnaderi commented Nov 26, 2023

@rlavolee By the way, are you finished by the PR? or are there other changes you want to make?
If everything's OK, we can merge it. I'm just curios about testing a snapshot version on some real projects to see the behavior before merging, which I'll do tomorrow.

@rlavolee
Copy link
Contributor Author

@hnaderi I'm done with it :) and eager to test it.

@hnaderi hnaderi merged commit d488ca7 into hnaderi:main Nov 26, 2023
9 of 10 checks passed
@hnaderi
Copy link
Owner

hnaderi commented Nov 26, 2023

@rlavolee The snapshot version should be published in a few minutes.
Thanks for the contribution! 🙏

@rlavolee
Copy link
Contributor Author

@hnaderi Your're welcome that was fun. Lepus is great !

Copy link
Contributor

@gnoireaux gnoireaux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 😉

And thanks for Lepus !

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.

3 participants