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

Return conn instead of chan from connection fun #52

Merged
merged 2 commits into from
Aug 16, 2022

Conversation

mfalt
Copy link
Contributor

@mfalt mfalt commented Apr 1, 2022

Might break some code that depends on this incorrect behavior.

mfalt and others added 2 commits April 1, 2022 16:25
Might break some code that depends on this incorrect behavior.
@tanmaykm
Copy link
Member

tanmaykm commented Apr 2, 2022

Thanks, this does seem correct.
Just want to walk through the code once to check if anything else is impacted before merging.

@nsslh
Copy link
Contributor

nsslh commented Aug 12, 2022

I am anxious about the impact of this change on existing code that passes the return value of connection into channel.

conn = connection(; ...)
chan = channel(conn, AMQPClient.UNUSED_CHANNEL, true)
basic_qos(chan, 0, 1, false; timeout = 10)
success, queue_name, ... = queue_declare(...)
... = basic_consume(chan, queue_name, msg -> consume_fn(chan, msg, routes))
fetch(chan.consumers[consumer_tag].receiver)

@mfalt
Copy link
Contributor Author

mfalt commented Aug 15, 2022

I think this specific case won't be a problem since the channel function seems to have been implemented to handle both connection and `channel``as input:

channel(c::MessageChannel, id::Integer) = channel(c.conn, id)

It would however break code like this:

conn = connection(; ...)
chan = channel(chan.conn, AMQPClient.UNUSED_CHANNEL, true)

So I think it warrants a bump to 0.5

@nsslh
Copy link
Contributor

nsslh commented Aug 15, 2022

So I think it warrants a bump to 0.5

If AMQPClient is using semantic versioning, then 1.0.0 is probably a safer choice, since it's a breaking change (for some).

@mfalt
Copy link
Contributor Author

mfalt commented Aug 15, 2022 via email

@nsslh
Copy link
Contributor

nsslh commented Aug 15, 2022

Semver doesn't require any compability for releases before 1.0.0

I had never noticed that. Thanks!

@nsslh
Copy link
Contributor

nsslh commented Aug 15, 2022

Fixes #51

@tanmaykm tanmaykm merged commit 865674f into JuliaComputing:master Aug 16, 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.

3 participants