Simplify the handle ordering API

[johnsimons]

Problem description

At the moment we have a very IMO unintuitive API to specify message ordering.

1. The user needs to implement ISpecifyMessageHandlerOrdering, eg

class EnsureOrdering : ISpecifyMessageHandlerOrdering
{
    public void SpecifyOrder(Order order)
    {
        order.Specify(First<H1>.Then<H2>());
    }
}

1. The user can also specify order via the config.LoadMessageHandlers overloads, eg:

config.LoadMessageHandlers(First<H1>.Then<H2>().AndThen<H3>().AndThen<H4>())

Proposal

• Deprecate all the above API;
• Introduce a new simple api to specify ordering;
• Doco (see Handler ordering, upgrade guide and snippets for it docs.particular.net#707)

New API

config.ExecuteTheseHandlersFirst(params Type[] handlerTypes)

[johnsimons]

@andreasohlund @indualagarsamy @SimonCropp thumbs up or down ?

[andreasohlund]

Since users might have ISpecifyMessageHandlerOrdering in separate dll's what would be the replacement?

Creating a INeedInitialization and call builder.InvokeTheseHandlersFirst from there?

[johnsimons]

U actually raise a good point, currently if the user has multiple
ISpecifyMessageHandlerOrdering
what happens? Last one wins?

On Wednesday, August 6, 2014, Andreas Öhlund notifications@github.com
wrote:

Since users might have ISpecifyMessageHandlerOrdering in separate dll's
what would be the replacement?

Creating a INeedInitialization and call builder.InvokeTheseHandlersFirst
from there?

—
Reply to this email directly or view it on GitHub
#2280 (comment)
.

[johnsimons]

Yes an INeedInitialization would work.

On Wednesday, August 6, 2014, Andreas Öhlund notifications@github.com
wrote:

Since users might have ISpecifyMessageHandlerOrdering in separate dll's
what would be the replacement?

Creating a INeedInitialization and call builder.InvokeTheseHandlersFirst
from there?

—
Reply to this email directly or view it on GitHub
#2280 (comment)
.

[andreasohlund]

They add up, and we throw if they are conflicting I think

On Wed, Aug 6, 2014 at 2:01 PM, John Simons notifications@github.com
wrote:

Yes an INeedInitialization would work.

On Wednesday, August 6, 2014, Andreas Öhlund notifications@github.com
wrote:

Since users might have ISpecifyMessageHandlerOrdering in separate dll's
what would be the replacement?

Creating a INeedInitialization and call builder.InvokeTheseHandlersFirst
from there?

—
Reply to this email directly or view it on GitHub
<
https://github.com/Particular/NServiceBus/issues/2280#issuecomment-51318473>

.

—
Reply to this email directly or view it on GitHub
#2280 (comment)
.

[johnsimons]

@SimonCropp you always have good argument in favour of removing code, can you chip in on these and lets make a decision.

[SimonCropp]

ok so while builder.InvokeTheseHandlersFirst(IEnumerable<Type>) is a little less type safe and a little less readable i think it is a better, and actually more discoverable, API. I know i have never once been able to use the First<H1>.Then<H2> without going and copying a sample from somewhere.

So a big +1 from me

[andreasohlund]

I'm +1 for doing it. But with all the other stuff we have going on can it wait to V6?

Sent from my iPhone

On 08 Aug 2014, at 03:07, Simon Cropp notifications@github.com wrote:

ok so while builder.InvokeTheseHandlersFirst(IEnumerable) is a little less type safe and a little less readable i think it is a better, and actually more discoverable, API. I know i have never once been able to use the First

.Then

without going and copying a sample from somewhere.

So a big +1 from me

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

[johnsimons]

yep

On 8 August 2014 15:49, Andreas Öhlund notifications@github.com wrote:

I'm +1 for doing it. But with all the other stuff we have going on can it
wait to V6?

Sent from my iPhone

On 08 Aug 2014, at 03:07, Simon Cropp notifications@github.com wrote:

ok so while builder.InvokeTheseHandlersFirst(IEnumerable) is a
little less type safe and a little less readable i think it is a better,
and actually more discoverable, API. I know i have never once been able to
use the First

.Then

without going and copying a sample from
somewhere.

So a big +1 from me

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

—
Reply to this email directly or view it on GitHub
#2280 (comment)
.

[andreasohlund]

cc @DavidBoike

[johnsimons]

@Particular/nservicebus this one is ready to be reviewed.
Not sure about the proposed method names ?

[danielmarbach]

@johnsimons How can we proceed with this? I think the method names are good.

[johnsimons]

@danielmarbach a bit busy with the SC retries, but all is left is merge and doco

[danielmarbach]

@johnsimons Decided to get rid of the IEnumerable overload because when you do this:

The compiler chooses the params overload and then Resharper says your definition is redundant.

                var handlerTypes = new []{ typeof(Saga1), typeof(Saga2)};
                c.ExecuteTheseHandlersFirst(handlerTypes);

picks also the params overload

When you want to pass in an IEnumerable it is low friction to just call ToArray() like this

                var handlerTypes = new List<Type>{ typeof(Saga1), typeof(Saga2)};
                c.ExecuteTheseHandlersFirst(handlerTypes.ToArray());

One less public API is good.

[danielmarbach]

@johnsimons hold your breath. I did a rebase.

[danielmarbach]

@johnsimons I did the doco. Please chime in for the review here Particular/docs.particular.net#707 If everything is good please go ahead and merge both

[SimonCropp]

Disagree. If u only do one method u shoud do the ienumerable one.

[danielmarbach]

@SimonCropp provide more context please. If not your reasoning cannot be understood.

[SimonCropp]

@danielmarbach the standard approach for taking in multiple items in an api is IEnumerable. i dont see a reason to deviate from that.

as for

One less public API is good.

While i agree with that in principle i dont think it applies in this case. i cant think of a scenario where we would change this api in a way that would cause extra pain to the consumer because we are taking the standard approach of "param and a enumerable overload". Effectively, in this case, it is one api with two manifestations for convenience. So i think leave it with a params and enumerable

[danielmarbach]

Yeah but did you read my comment how it looks from the usage perspective?

I don't feel strong about it. You have more experience with those sort of APIs than I have. I just felt weird when coding against the API. Based on that hunch I removed the enumerable overload.

[SimonCropp]

@danielmarbach i am confused

the regular use of this api is to pass in a small number of types known at compile time. so the usage would not be

   c.ExecuteTheseHandlersFirst(new []{ typeof(Saga1), typeof(Saga2)});

but

   c.ExecuteTheseHandlersFirst(typeof(Saga1), typeof(Saga2));

[danielmarbach]

Ergo we don't need the enumerable overload ;)

[SimonCropp]

there is not tax in this extra api #2280 (comment)

and there is some benefit. ie no need for a toarray. ergo we should have both

[johnsimons]

@danielmarbach the only reason I added the IEnumerable overload was so that if the user already has an enumeration of handler types, they can just pass that in, eg:

IEnumerable<Type> handlersToExecuteFirst = GetListOfHandlers();
c.ExecuteTheseHandlersFirst(handlersToExecuteFirst);

This way there is no need to allocate an extra array.

Does this make sense ?

Also in your example, R# is just suggesting an optimisation based on static code analysis, and yes in that specific case it makes sense to do what R# suggests, but in the example above there is no optimisation.

[danielmarbach]

@johnsimons @SimonCropp reverted back to have both APIs.