Polyconseil / aioamqp

AMQP implementation using asyncio
Other
280 stars 88 forks source link

Message Properties: 'type' property #201

Open notmeta opened 5 years ago

notmeta commented 5 years ago

Since the new release aioamqp uses pamqp to encode/decode frames.

This also includes properties, but a subtle difference in naming for the 'type' property caused problems when I upgraded to the latest version.

aioamqp uses 'type', whereas pamqp uses 'message_type'.

This is not immediately obvious as the current aioamqp docs still specify 'type', see https://github.com/Polyconseil/aioamqp/blob/master/docs/api.rst#consuming-messages

Example:

# aioamqp 0.12.0
await conn.channel.publish(msg.payload,
                                               exchange_name=exchange_name,
                                               routing_key=routing_key,
                                               properties={'type': ...}  # fine
                                               )

# aioamqp 0.13.0
# returns UNEXPECTED_FRAME - expected content header for class 60, got non content header frame instead
await conn.channel.publish(msg.payload,
                                               exchange_name=exchange_name,
                                               routing_key=routing_key,
                                               properties={'type': ...}  # invalid
                                               )

# aioamqp 0.13.0
await conn.channel.publish(msg.payload,
                                               exchange_name=exchange_name,
                                               routing_key=routing_key,
                                               properties={'message_type': ...}  # fine
                                               )

I suggest one or more of the following changes:

dzen commented 5 years ago

Hello @notmeta.

Thanks for all your viable suggestions. Imho there is two things:

First, we should rename type with message_type which will help us remove the disabled pylint and fix the issue quickly.

In a second hand, we should probably use the aioamqp.Properties to be used in channel.publish instead for a cleaner and self describing API, not an obscure dict argument.

What do you think ?

notmeta commented 5 years ago

@dzen

Both sound good to me!

dzen commented 5 years ago

@notmeta I may have the time to propose the first part.

notmeta commented 5 years ago

@dzen I may find some time to work on the second bit, if you're unable to.

Also I had a thought: since this is going to be a breaking change, how should the version upgrade go? Are you guys wanting to follow semantic versioning, or should it just be implied that as this is pre-1.0.0 that breaking changes may happen at any time?

dzen commented 5 years ago

I think since we do not released a 1.x version, we may change the API. I think we did it already.

dzen commented 5 years ago

@notmeta : do you want a 0.13.x release ?

notmeta commented 5 years ago

@dzen no rush on my end (as I've already fixed it in my codebase) but I'm sure others might encounter issues if they use that property.