Skip to content
This repository has been archived by the owner on Nov 18, 2020. It is now read-only.

Propagate x-headers in SEND and SUBSCRIBE to queue.declare #27

Merged
merged 4 commits into from
Aug 20, 2015

Conversation

priviterag
Copy link
Contributor

Fixes #13.

x-dead-letter-exchange, x-dead-letter-routing-key and x-max-priority
parsing in SEND and SUBSCRIBE

ensure_endpoint(Direction, Endpoint, _Frame, Channel, State) ->
rabbit_routing_util:ensure_endpoint(Direction, Channel, Endpoint, State).
{_, _, Headers, _} = Frame,
Copy link
Member

Choose a reason for hiding this comment

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

You can destructure right in the head argument list.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also in this case Frame is unbound, since the argument above is _Frame.

Copy link
Contributor

Choose a reason for hiding this comment

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

Scratch my comment, I misunderstood the diff

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please, could you provide an example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@michaelklishin or better, what is the idiomatic erlang way?

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of having something like: ensure_endpoint(Direction, Endpoint, Frame, Channel, State) you can do ensure_endpoint(Direction, Endpoint, {_, _, Headers, _} = Frame, Channel, State)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@videlalvaro clever! thanks. and in the function do I get both Headers and Frame in scope?

Copy link
Member

Choose a reason for hiding this comment

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

Álvaro's example is what I had in mind.

build_argument(?HEADER_X_MAX_PRIORITY, Val) ->
{list_to_binary(?HEADER_X_MAX_PRIORITY), long, list_to_integer(string:strip(Val))};
build_argument(?HEADER_X_MESSAGE_TTL, Val) ->
{list_to_binary(?HEADER_X_MESSAGE_TTL), long, list_to_integer(string:strip(Val))}.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this function can be refactored to not repeat almost the same function body in the different clauses. What changes is the type of the argument, long, longstr, and so on.

Copy link
Member

Choose a reason for hiding this comment

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

it's very declarative and small, though. Without proper macroexpansion capabilities in Erlang I'd leave it as is.

Copy link
Member

Choose a reason for hiding this comment

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

Key and value conversion to binary can probably happen before build_argument is called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would leave as it is. As you said, it's declarative and small. IMO, key and value conversion is exactly the responsibility of the 'build_argument' function, in fact it's different for each header.

@michaelklishin michaelklishin self-assigned this Aug 20, 2015
@michaelklishin michaelklishin added this to the n/a milestone Aug 20, 2015
@michaelklishin michaelklishin changed the title added queue properties parsing in SEND and SUBSCRIBE Propagate x-headers in SEND and SUBSCRIBE to queue.declare Aug 20, 2015
michaelklishin added a commit that referenced this pull request Aug 20, 2015
Propagate x-headers in SEND and SUBSCRIBE to queue.declare
@michaelklishin michaelklishin merged commit 58b3b50 into stable Aug 20, 2015
@dumbbell dumbbell deleted the rabbitmq-stomp-13 branch January 2, 2018 16:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants