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.

Sign up for GitHub

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
Merged

Add CreateConnection overload to IConnectionFactory #325

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions projects/client/RabbitMQ.Client/src/client/api/IConnectionFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
using System;
using System.Collections.Generic;
using System.Threading.Tasks;
using RabbitMQ.Client.Exceptions;

namespace RabbitMQ.Client
{
Expand Down Expand Up @@ -135,6 +136,21 @@ public interface IConnectionFactory
/// <returns></returns>
IConnection CreateConnection(IList<string> hostnames, String clientProvidedName);

/// <summary>
/// Create a connection using a list of endpoints. By default each endpoint will be tried
/// in a random order until a successful connection is found or the list is exhausted.
/// The selection behaviour can be overriden by configuring the EndpointResolverFactory.
/// </summary>
/// <param name="endpoints">
/// List of endpoints to use for the initial
/// connection and recovery.
/// </param>
/// <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 •
edited
Loading

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

IConnection CreateConnection(IList<AmqpTcpEndpoint> endpoints);

/// <summary>
/// Advanced option.
///
Expand Down