-
Notifications
You must be signed in to change notification settings - Fork 591
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
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.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I raised #332
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