Raise Non-EventArgs events

[galaktor]

The Raise<>.With syntax is typed to events with the EventArgs convention.

There are many cases where I use events with custom delegate types, like such

public delegate void MyDelegate(MyCargo c);
// ...
public event MyDelegate Foo;

Unfortunately I cannot raise such an event on a Fake with the given Raise<> API.

A way to raise any type of delegate/cargo would be very useful; meaning if Raise<> took a custom/generic delegate signature and didn't force the (sender,EventArgs) pattern.

The use case I imagine (or, really, expected at first) was this (continuing the above delegate example):

var f = A.Fake<IFireEvents>();

f.Foo += Raise.With<MyDelegate>(new Cargo()).Now;

or maybe

f.Foo += Raise.With<MyCargo>(new Cargo()).Now;

where, in the latter case, Raise would expose an Action<TCargo> or similar.

Not sure how much of this is really possible, but depending on how desperate I get I might try to hack it up myself in a fork.
However, I'm sure you guys know what to do in this case much better than I do :-)

Thanks for the great tool!

[msevestre]

+1 for that feature. I just ported an application from using Rhino.Mocks to FakeItEasy. That the only feature I am missing so far.

With that kind of implementation, will it be possible to Raise an Action ?
For instance the object just defines an event Action Changed;

[philippdolder]

With my current understanding of FakeItEasy internals it is not that easy to implement this feature without breaking existing stuff. I definitely need to take a deeper look inside, but I'm lacking spare time at the moment :-(
In case the world doesn't end on friday 21st, I hope I can get some time to push FakeItEasy a step forward during xmas holidays

[philippdolder]

Duplicate of issue #26

[philippdolder]

Just to say:
This particular feature is the next thing I will work on for FakeItEasy. But please be aware that it will take some time for me to complete it.

[msevestre]

I'll be happy to help if there is a consensus on how that feature should be
implemented

On Thu, Jan 10, 2013 at 4:59 PM, Philipp Dolder notifications@github.comwrote:

Just to say:
This particular feature is the next thing I will work on for FakeItEasy.
But please be aware that it will take some time for me to complete it.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/30#issuecomment-12103621.

[philippdolder]

I still have to think about the best syntax and details how it should be done yet. I think it should just be possible to raise any kind of delegate.

Thanks, msevestre. I will come back to you when I know more

[patrik-hagne]

How is this done in NSub? I think they did borrow the Raise-syntax but I haven't really looked at it.

[philippdolder]

I'm starting to spike this one

[msevestre]

Awesome!!
On Apr 25, 2013 9:25 AM, "Philipp Dolder" notifications@github.com wrote:

I'm starting to spike this one

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/30#issuecomment-17006373
.

[galaktor]

great

[philippdolder]

from what I've found out so far it's not possible (with my knowledge and understanding) to use the same approach as we use to raise normal events to implement the raising of delegates (and actions).

The problem is the Now method in the Raise which I cannot rebuild when I want to have any delegate instead of EventHandler.

thoughts?

[philippdolder]

@patrik-hagne

How is this done in NSub? I think they did borrow the Raise-syntax but I haven't really looked at it.

NSub doesn't use the Now. They just have Raise.With() and that raises the event already.

I'm currently having an issue (see above) with the Now to find a way to implement non EventHandler constrained event raising

[adamralph]

@philippdolder if you push your changes onto a branch in your fork I'll take a look.

[patrik-hagne]

Could we do something like Raise.NonEventDelegateWith()?

Maybe we could even hide that from intellisense (EditorBrowsableAttribute) so that it can be used but it's not discoverable so that we don't promote this worst practice.

[philippdolder]

@patrik-hagne I wouldn't hide it away. If people don't see it, they won't find it. But maybe that feature should be part of a kind of additional package, because we don't want to encourage people to do it that way?

@FakeItEasy/owners do we agree that using events with custom delegates that are not (object sender, EventArgs e) are bad practice? I'm personally not strongly opinionated on that.

[adamralph]

I think it is bad practice but sometimes you just need to fake that nasty third party dependency...

Whilst I sympathise with the motivation to hide it from Intellisense, for most people !Intellisense == !Exists. No one will ever know about it and I bet we'll just get same issue raised again :-)

[philippdolder]

So. What do you think about creating a separate package then? We should not do it in "Core" and then remove it later. I'd prefer having a 2nd nuget package where we put "things you shouldn't do, but sometimes have to" (see also #90). FakeItEasy.LegacyExtensions comes to my mind for a name. Or something more general where we can put everything that is not part of the core framework?

[msevestre]

Hi all,

I do not agree that this is bad practice. In my opinion, the fact that an
event signature should always be the same is an anti pattern. There has
been a lot of discussion 4-5 years ago on that subject and I really don't
see it as a black and white case. There are cases where it makes sens,
other when it doesn't. It's like forcing all public methods to have the
same signature. Why would you impose that?

There are so many use cases where the code looks easier to read when you
can just use an action, or pass explicitly the one parameter you may
need. Think about a Changed() event. Why on earth would I need an EventArgs?

I truly think it belongs into the core package. Why should we deal with yet
another extra assembly just to write specs?

On Tue, Apr 30, 2013 at 2:08 PM, Philipp Dolder notifications@github.comwrote:

So. What do you think about creating a separate package then? We should
not do it in "Core" and then remove it later. I'd prefer having a 2nd nuget
package where we put "things you shouldn't do, but sometimes have to" (see
also #90 #90).
FakeItEasy.LegacyExtensions comes to my mind for a name. Or something more
general where we can put everything that is not part of the core framework?

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/30#issuecomment-17223276
.

[patrik-hagne]

I would argue that that would make it even less discoverable. I'd say it'd be better for it to be hidden but when discovered (through the wonders of Google) it'd be accessible directly rather than having to download something.

Can we change the syntax to be exactly what NSub does?

[patrik-hagne]

@msevestre It goes against the coding guidlines for the framework and expectations of "consumers" so it is bad practice. You can always use whatever delegate for callbacks, just don't tag it with the event keyword since that will break in so many scenarios.

I do agree that the guidlines shouldn't have been that way, but they are.

[msevestre]

I haven't seen anything break because the event does not respect the
framework guideline. There is a reason why FxCop, NDepend etc are not
checking this rule anymore by default in their analysis. A guideline that
is more than 10 years old has all right to be wrong...that would not be the
first time

On Tue, Apr 30, 2013 at 2:21 PM, Patrik Hägne notifications@github.comwrote:

@msevestre https://github.com/msevestre It goes against the coding
guidlines for the framework and expectations of "consumers" so it is bad
practice. You can always use whatever delegate for callbacks, just don't
tag it with the event keyword since that will break in so many scenarios.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/30#issuecomment-17223765
.

[adamralph]

At the moment my view is that we should add a new method group in the main package, visible to Intellisense, just like NSub do.

NSub has method groups:

• Raise.EventWith<TEventArgs> where TEventArgs : EventArgs
• Raise.Event<THandler> // unconstrained

FIE has:

• Raise.With<TEventArgs> where TEventArgs : EventArgs

I think we can just add Raise.Event<THandler> // unconstrained.

[msevestre]

Great idea! +1
On May 1, 2013 4:36 AM, "Adam Ralph" notifications@github.com wrote:

At the moment my view is that we should add a new method group in the main
package, visible to Intellisense, just like NSub do.

NSub has method groups:

• Raise.EventWith where TEventArgs : EventArgs
• Raise.Event // unconstrained

FIE has:

• Raise.With where TEventArgs : EventArgs

I think we can just add Raise.Event // unconstrained.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/30#issuecomment-17273093
.

[adamralph]

I think really I just formalised what @patrik-hagne suggested ;-)

[philippdolder]

@adamralph sounds reasonable to me.

[patrik-hagne]

Sounds good, what would be the exact signature of Raise.Event<THandler> be?

[adamralph]

In NSub it's

public static DelegateEventWrapper<THandler> Event<THandler>(params object[] arguments)

I guess FIE could do something very similar, e.g.

public static DelegateRaise<THandler> Event<THandler>(params object[] arguments)

where DelegateRaise<T> would be the unconstrained equivalent of Raise<TEventArgs>.

NSub's DelegateEventWrapper<THandler> accepts the arguments array in it's constructor and reflectively matches them up with the delegate's arguments, throwing an exception if things don't match up.

If we feel that this is getting too messy with With and Event then perhaps we could even deprecate With and come up with some new method names.

[adamralph]

We could even think about deprecating the .Now syntax. NSub gets around this cleverly with an implicit operator override on it's EventHandlerWrapper<TEventArgs> and DelegateEventWrapper<T>. Worth taking a look at https://github.com/nsubstitute/NSubstitute/blob/master/Source/NSubstitute/Raise.cs

[blairconrad]

Now planning for 2.0.0 release. We will remove Now and Go, and events of type RoutedEventHandler (basically any non-generic eventhandler with two args, an object and a type that extends EventArgs) will require a typeparam indicating the handler type. This is a breaking change.

[blairconrad]

An additional breaking change that I think we should consider:

having Raise.With(null, args) actually raise with the sender being null, instead of "self". This will be consistent with the behaviour for delegate-based events, where parameters will be passed through as-is. And there's already a "self" convenience method, Raise.With(args).

[adamralph]

Agree.

[blairconrad]

This issue has been fixed in release 2.0.0.