Skip to content
This repository has been archived by the owner on Aug 8, 2022. It is now read-only.

Allow binding to multiple routing keys #4

Merged
merged 3 commits into from
Dec 27, 2018

Conversation

thyandrecardoso
Copy link

This should allow bindings to multiple routing keys, tackling sstone#68 and further working on sstone#73.

Copy link
Member

@jcazevedo jcazevedo left a comment

Choose a reason for hiding this comment

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

Looks good! I just pointed out some minor style issues.

@@ -75,7 +75,7 @@ object Amqp {
*/
case class ChannelParameters(qos: Int, global: Boolean = false)

case class Binding(exchange: ExchangeParameters, queue: QueueParameters, routingKey: String)
case class Binding(exchange: ExchangeParameters, queue: QueueParameters, routingKeys: Set[String])
Copy link
Member

Choose a reason for hiding this comment

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

I think it's preferable to use varargs instead of a Set here, as it will make the declaration of Bindings simpler. The same applies to QueueBind below.

Copy link
Author

Choose a reason for hiding this comment

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

The approach discussed offline (re-ordering arguments) won't work because of the default argument value in:
case class QueueBind(queue: String, exchange: String, routingKeys: Set[String], args: Map[String, AnyRef] = Map.empty) extends Request

This:
case class QueueBind(queue: String, exchange: String, args: Map[String, AnyRef] = Map.empty, routingKeys: String*) extends Request

is not allowed.

I'm thinking it might be best to leave this as is. Any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

Let's leave it as is then.

src/test/scala/com/github/sstone/amqp/ConsumerSpec.scala Outdated Show resolved Hide resolved
src/test/scala/com/github/sstone/amqp/ConsumerSpec.scala Outdated Show resolved Hide resolved
src/test/scala/com/github/sstone/amqp/ConsumerSpec.scala Outdated Show resolved Hide resolved
src/test/scala/com/github/sstone/amqp/ConsumerSpec.scala Outdated Show resolved Hide resolved
src/test/scala/com/github/sstone/amqp/ConsumerSpec.scala Outdated Show resolved Hide resolved
Co-Authored-By: belerophon <thyandrecardoso@hotmail.com>
Copy link
Member

@jcazevedo jcazevedo left a comment

Choose a reason for hiding this comment

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

LGTM

@jcazevedo jcazevedo merged commit 3b0cd3b into sf-master Dec 27, 2018
@jcazevedo jcazevedo deleted the feature/allow-binding-multiple-routing-keys branch December 27, 2018 14:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants