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

Add CreateConnection overload to IConnectionFactory #325

Merged
merged 1 commit into from
Jun 1, 2017

Conversation

pardahlman
Copy link
Contributor

The method was implemented in ConnectionFactory, so the only thing done was to pull it up to the interface.

This fixes #324

The method was implemented in ConnectionFactory, so
the only thing done was to pull it up to the interface.
This fixes rabbitmq#324
@michaelklishin michaelklishin merged commit 3944d66 into rabbitmq:master Jun 1, 2017
/// <returns>Open connection</returns>
/// <exception cref="BrokerUnreachableException">
/// When no hostname was reachable.
/// </exception>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should exceptions be documented on interfaces, or only on implementations? I've never been sure, and I've heard arguments both ways.

I guess you could also argue that some of the content in the summary tag is implementation specific.

Copy link
Member

@michaelklishin michaelklishin Jun 1, 2017

Choose a reason for hiding this comment

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

In this case I'm leaning towards a "yes". BrokerUnreachableException is something I can see mock implementations throwing.

Copy link
Contributor

Choose a reason for hiding this comment

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

@adamralph what would Liskov say? :) I suspect yes as it is very much something you'd have to account for in making any implementation "replacable" without changes to the consuming code. That said specifying exactly what exceptions should be allowable for any one implementation isn't always easy to do up front.

I agree that the summary comment is implementation specific in this case it depends on the implementation of IEndpointResolver.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kjnilsson good point. I guess, from that point of view, the exception docs can be considered part of the interface definition, requiring that every implementation behaves that way. The concrete way to do that would be something like checked exceptions in Java, but that is its own can of worms. :-)

I agree that the summary comment is implementation specific in this case it depends on the implementation of IEndpointResolver.

Do you think it's worth changing the comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it should probably be changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I raised #332

@kjnilsson
Copy link
Contributor

I've created a pre-release that includes change: https://www.nuget.org/packages/RabbitMQ.Client/5.0.1-pre3

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.

Consider expanding IChannelFactory with method from ChannelFactory
4 participants