Add CreateConnection overload to IConnectionFactory #325
Conversation
The method was implemented in ConnectionFactory, so the only thing done was to pull it up to the interface. This fixes rabbitmq#324
| /// <returns>Open connection</returns> | ||
| /// <exception cref="BrokerUnreachableException"> | ||
| /// When no hostname was reachable. | ||
| /// </exception> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
In this case I'm leaning towards a "yes". BrokerUnreachableException is something I can see mock implementations throwing.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
Yes it should probably be changed.
|
I've created a pre-release that includes change: https://www.nuget.org/packages/RabbitMQ.Client/5.0.1-pre3 |
The method was implemented in ConnectionFactory, so the only thing done was to pull it up to the interface.
This fixes #324