Use optionals

[MisinformedDNA]

Make parameters optional.

So instead of this

    public RabbitMQ.Client.QueueDeclareOk QueueDeclare()
    {
      return this.QueueDeclare("", false, true, true, (IDictionary<string, object>) null);
    }

    public RabbitMQ.Client.QueueDeclareOk QueueDeclare(string queue, bool durable, bool exclusive, bool autoDelete, IDictionary<string, object> arguments)
    {
      RabbitMQ.Client.QueueDeclareOk queueDeclareOk = this.m_delegate.QueueDeclare(queue, durable, exclusive, autoDelete, arguments);
      RecordedQueue q = new RecordedQueue(this, queueDeclareOk.QueueName).Durable(durable).Exclusive(exclusive).AutoDelete(autoDelete).Arguments(arguments).ServerNamed(string.Empty.Equals(queue));
      this.m_connection.RecordQueue(queueDeclareOk.QueueName, q);
      return queueDeclareOk;
    }

consider something like

    public RabbitMQ.Client.QueueDeclareOk QueueDeclare(string queue = "", bool durable = false, bool exclusive true, bool autoDelete = true, IDictionary<string, object> arguments = null)
    {
      RabbitMQ.Client.QueueDeclareOk queueDeclareOk = this.m_delegate.QueueDeclare(queue, durable, exclusive, autoDelete, arguments);
      RecordedQueue q = new RecordedQueue(this, queueDeclareOk.QueueName).Durable(durable).Exclusive(exclusive).AutoDelete(autoDelete).Arguments(arguments).ServerNamed(string.Empty.Equals(queue));
      this.m_connection.RecordQueue(queueDeclareOk.QueueName, q);
      return queueDeclareOk;
    }

This way we can just set the parameters we are interested in and just accept the defaults for the rest.

[michaelklishin]

There have been similar proposals in the past. We'd be happy to include something like this but it will not replace the API we already have: breaking it is not an option.

Named parameter arguments in new-ish C# versions has been a decent middle ground recently.

On 29 sept 2015, at 4:33, Dan Friedman notifications@github.com wrote:

Make parameters optional.

So instead of this,

public RabbitMQ.Client.QueueDeclareOk QueueDeclare()
{
  return this.QueueDeclare("", false, true, true, (IDictionary<string, object>) null);
}

public RabbitMQ.Client.QueueDeclareOk QueueDeclare(string queue, bool durable, bool exclusive, bool autoDelete, IDictionary<string, object> arguments)
{
  RabbitMQ.Client.QueueDeclareOk queueDeclareOk = this.m_delegate.QueueDeclare(queue, durable, exclusive, autoDelete, arguments);
  RecordedQueue q = new RecordedQueue(this, queueDeclareOk.QueueName).Durable(durable).Exclusive(exclusive).AutoDelete(autoDelete).Arguments(arguments).ServerNamed(string.Empty.Equals(queue));
  this.m_connection.RecordQueue(queueDeclareOk.QueueName, q);
  return queueDeclareOk;
}

consider something like

public RabbitMQ.Client.QueueDeclareOk QueueDeclare(string queue = "", bool durable = false, bool exclusive true, bool autoDelete = true, IDictionary<string, object> arguments = null)
{
  RabbitMQ.Client.QueueDeclareOk queueDeclareOk = this.m_delegate.QueueDeclare(queue, durable, exclusive, autoDelete, arguments);
  RecordedQueue q = new RecordedQueue(this, queueDeclareOk.QueueName).Durable(durable).Exclusive(exclusive).AutoDelete(autoDelete).Arguments(arguments).ServerNamed(string.Empty.Equals(queue));
  this.m_connection.RecordQueue(queueDeclareOk.QueueName, q);
  return queueDeclareOk;
}

This way we can just set the parameters we are interested in and just accept the defaults for the rest.

—
Reply to this email directly or view it on GitHub.

[MisinformedDNA]

Nothing would break. In the case of pre-existing code, the optional values would already be provided, so there is no difference to the developer.

[michaelklishin]

@MisinformedDNA so your suggestion is to simply use the default argument values? That makes a lot of sense. We may be short on time to fit this into the 3.6.0 release but after that, I'm all for it.

[kjnilsson]

I have started work on this issue and it is mostly straightfoward, however for methods such as BasicConsume that would benefit from optionals there is a slight issue that the last method argument is the non-optional IBasicConsumer. Optional parameters need to appear after all required parameters which gives us two choices:

1. Change the Signature of BasicConsume (and others if/when found) so that all the required parameters come first. This means we are breaking source compatibility on a very commonly used method.
2. We don't use optionals for these methods.

@michaelklishin WDYT?

[michaelklishin]

@kjnilsson we can introduce a new method, BasicConsumeWith, which takes the consumer as the first argument.

[michaelklishin]

…and leave the current method alone.

[kjnilsson]

We could although that introduces more API and code to maintain. It seems I am able to change the method signature in IModel and still retain the old api using extension methods. See this wip commit: a66e9ee

This looks to me like a potential way forward.

[bording]

Optional parameters are often not something you want introduce on a public API, because making any changes to those methods results in a binary breaking change if you aren't incredibly careful!

http://haacked.com/archive/2010/08/10/versioning-issues-with-optional-arguments.aspx/

It is possible to do it without causing breaks, but it requires following some very strict rules. For example, take a look at the Roslyn guidelines:

https://github.com/dotnet/roslyn/blob/master/docs/Adding%20Optional%20Parameters%20in%20Public%20API.md

Multiple overloads are often a better way to go for a public API.

[michaelklishin]

This assumes breaking binary changes are a no-no. I don't think so. The 4.0 release is a great opportunity to introduce breaking changes to a 10 year old codebase
that supported .NET 1.1 (!) just 3 years or so ago.

On 27 jul 2016, at 20:48, Brandon Ording notifications@github.com wrote:

Optional parameters are often not something you want introduce on a public API, because making any changes to those methods results in a binary breaking change if you aren't incredibly careful!

http://haacked.com/archive/2010/08/10/versioning-issues-with-optional-arguments.aspx/

It is possible to do it without causing breaks, but it requires following some very strict rules. For example, take a look at the Roslyn guidelines:

https://github.com/dotnet/roslyn/blob/master/docs/Adding%20Optional%20Parameters%20in%20Public%20API.md

Multiple overloads are often a better way to go for a public API.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[bording]

This assumes breaking binary changes are a no-no. I don't think so. The 4.0 release is a great opportunity to introduce breaking changes to a 10 year old codebase

Sure, moving up to a new major release is definitely the time to be considering breaking changes. I'm thinking more about what happens with releases past that point. If you introduce optionals in 4.0, then you now take on the maintenance cost of avoiding breaks as you continue to modify the API.

[kjnilsson]

Thanks @bording for pointing out the issues with optionals and binary compatibility. I hadn't considered that. It is unlikely that the API is going to change significantly going forward but I agree using optionals in IModel may not be the best approach from a maintenance POW.

We could move the signatures with optionals to extension methods as these wouldn't need to change if the underlying API method had to evolve. (Any parameter type change or removal would be a breaking change anyway and would require a major version bump). This would also allow us to reduce the surface area of IModel which I think is a good thing. I am not keen on convenience overloads in interface contracts and would like to see this concern separated out if possible.

[bording]

@kjnilsson For interfaces, a good approach I've seen in the past is to only have the the method with all the parameters on the interface itself, and then all of the convenience overloads are extension methods, which lets all implementations of the interface get them for free. I think this what you're suggesting above?

Of course, versioning interfaces brings it own set of challenges, regardless of optional parameters, because any changes there can be both compile and binary breaks, depending on the change.