Raise Non-EventArgs events about fakeiteasy HOT 59 CLOSED

+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;

from fakeiteasy.

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

from fakeiteasy.

Duplicate of issue #26

from fakeiteasy.

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.

from fakeiteasy.

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 [email protected]:

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.

from fakeiteasy.

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

from fakeiteasy.

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

from fakeiteasy.

I'm starting to spike this one

from fakeiteasy.

Awesome!!
On Apr 25, 2013 9:25 AM, "Philipp Dolder" [email protected] wrote:

I'm starting to spike this one

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

from fakeiteasy.

great

from fakeiteasy.

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?

from fakeiteasy.

@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

from fakeiteasy.

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

from fakeiteasy.

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.

from fakeiteasy.

@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.

from fakeiteasy.

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 :-)

from fakeiteasy.

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?

from fakeiteasy.

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 [email protected]:

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
.

from fakeiteasy.

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?

from fakeiteasy.

@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.

from fakeiteasy.

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 [email protected]:

@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
.

from fakeiteasy.

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.

from fakeiteasy.

Great idea! +1
On May 1, 2013 4:36 AM, "Adam Ralph" [email protected] 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
.

from fakeiteasy.

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

from fakeiteasy.

@adamralph sounds reasonable to me.

from fakeiteasy.

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

from fakeiteasy.

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.

from fakeiteasy.

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

from fakeiteasy.

+1 from my side for deprecating the .Now. In my experience quite a couple of people find it hard to raise events, and they often forget to type .Now and wonder why it doesn't compile.
But we only should do it if we deprecate the With and come up with new names for regular and delegate events. Otherwise we confuse people when we change the existing Raise.With syntax to no longer require the .Now.

NSub looks neat. Though I didn't have a deep look inside.
If we agree that @adamralph's suggestion is a good start I will continue my spike towards that direction.

from fakeiteasy.

Yeah, I like what they did in NSub, the "Now"-method was the only thing I came up with at the time, so if this works for all the same cases that the current method does then I'm all for going that route and deprecating the now-version.

from fakeiteasy.

Hey guys,
What is the status on that?
Thanks

from fakeiteasy.

@msevestre I'm currently working on a spike to find out how we can build this feature in and being backwards compatible at the same time.

from fakeiteasy.

Assigned P2. It's an old one but it keeps cropping up and I really think this part of the API should be improved. I think we may be lagging behind other frameworks here.

from fakeiteasy.

Bumping up to P1. We've had yet more complaints/enquires about this recently:

• https://twitter.com/hmemcpy/status/423840328744189952
• https://twitter.com/dhelper/status/425619457340604416

from fakeiteasy.

another query https://twitter.com/chrissie1/status/427821109866557441

from fakeiteasy.

@philippdolder, I know you're

1. invested in this feature, and
2. busy

Is there something I can do to help? I don't mind being sent off on random errands for research or spiking or provisions.

from fakeiteasy.

@FakeItEasy/owners I will have my annual talk with my manager this friday where I will plan the goals for this year. I hope to convince him of planning this feature for Q1 2014.

What I found in my current spike so far is that it seems to be almost impossible to introduce the new concept without breaking the old one. This means that we probably can't switch over "softly" e.g. deprecating the old way in one release and removing it in the next. We ought to do it the hard way and release 2.0.

@blairconrad what's ur time zone? I would appreciate if we could have a skype/teamviewer/pair programming session next week (end). mine is UTC+1

from fakeiteasy.

The entire concept of working on open source projects at work is alien to me. I wish it wasn't though 😉

I'm not sure if this is feasible but just throwing it out there: could we have an entire new API for events, parallel to the old one, which we could leave completely untouched?

from fakeiteasy.

As far as my insights and understanding go, the problem isn't the API per se. I think the implementation of the internals will be far more complex when supporting both mechanisms at the same time. Probably I just didn't see how to achieve that without touching the old event firing stuff as I didn't spend a lot of time in this specific direction.

from fakeiteasy.

@philippdolder, I'm in UTC-5. I'll contact you outside this issue regarding schedules. Sounds like fun.

from fakeiteasy.

I got budget from @bbvch for this issue. Currently, I'm a bit under pressure in my projects, but I should find time in May to work on this issue.

from fakeiteasy.

Awesome news @philippdolder.

Big kudos to @bbvch for having the foresight to fund OSS dev!

from fakeiteasy.

I ran into this issue and it was a deal breaker for my implementation, we have a public API that exposes an event that does not conform to the EventArgs guidelines and I can't go about changing that and breaking our consumers.

While the below method is not an ideal solution to this issue, it will let us keep using FIE until an official solution is provided in the framework. Leaving it with the hope that it helps others who hit this in the mean time.

        private void FireEvent(object fake, AgentEvent evt)
        {
            string evtName = "AgentEvtArrived";
            string add = string.Format("add_{0}", evtName);
            string rem = string.Format("remove_{0}", evtName);
            List<EventArrivedDelegate> wiredHandlers = new List<EventArrivedDelegate>();

            foreach (ICompletedFakeObjectCall call in  Fake.GetCalls(fake).Where(c => c.Method.Name == add || c.Method.Name == rem))
            {
                EventArrivedDelegate arg = call.Arguments[0] as EventArrivedDelegate;
                if (call.Method.Name.StartsWith("add_"))
                    wiredHandlers.Add(arg);
                else if (call.Method.Name.StartsWith("remove_"))
                    wiredHandlers.Remove(arg);
            }

            foreach (EventArrivedDelegate del in wiredHandlers)
                del.Invoke(evt);
        }

from fakeiteasy.

@akrock that does look like a handy workaround until we can get non-EventArgs-raising baked in. Thanks for sharing.

from fakeiteasy.

@akrock could you show how you would use that method?
Thanks

from fakeiteasy.

Sure. I changed it to use the example from the first post as well.

You could make this more generic by not casting to the specific delegate type but you will suffer a performance hit from calling DynamicInvoke instead of Invoke, which is why I chose to keep it specific to the actual delegate.

// An object that is being faked with Interface IFireEvents
public delegate void MyDelegate(MyCargo c);
public event MyDelegate Foo;
//....

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

f.Foo += (cargo) => 
    {
        Console.WriteLine("I got cargo.."); 
    };

FireEvent(f, new MyCargo());


//.....
private void FireEvent(object fake, MyCargo evt)
{
     string evtName = "Foo";
     string add = string.Format("add_{0}", evtName);
     string rem = string.Format("remove_{0}", evtName);
     List<MyDelegate> wiredHandlers = new List<MyDelegate>();

     foreach (ICompletedFakeObjectCall call in  Fake.GetCalls(fake).Where(c => c.Method.Name == add || c.Method.Name == rem))
     {
          MyDelegate arg = call.Arguments[0] as MyDelegate;
          if (call.Method.Name.StartsWith("add_"))
              wiredHandlers.Add(arg);
          else if (call.Method.Name.StartsWith("remove_"))
              wiredHandlers.Remove(arg);
     }

     foreach (MyDelegate del in wiredHandlers)
         del.Invoke(evt);
}

from fakeiteasy.

@FakeItEasy/owners, I was finally able to make a commit to show you what my problem is in implementing this one (atm I'm just trying to get rid of Now).

see my branch: https://github.com/philippdolder/FakeItEasy/tree/30-raise-non-eventargs-events

I created the NewEventRule and changed Raise.cs for the new syntax.
See RaiseTests.Raising_with_arguments_only_should_raise_event_with_specified_arguments to get started.

You might want to set a breakpoint in Raise.cs:71 and in NewEventRule.cs:30 if you debug that test you should see what's my problem.

I'm not able to raise the event at the location where I have all required information (e.g. EventArgs are missing when I would be able to raise an event. Where I have the EventArgs I don't see a way to raise an event).

Ideas?

from fakeiteasy.

Hi, @philippdolder. I'm not trying to ignore you. Had intended to go through this this morning, but got stuck in the JabbR room helping a client. I will take a look sometime soon!

from fakeiteasy.

@blairconrad no worries. I only find time slots once in a while. take your time

from fakeiteasy.

@FakeItEasy/owners, in #352 I have an implementation that supports:

// existing in 1.25.0
TypeWithEvent.EventHandler += Raise.With(raisedWithArgs).Now;
TypeWithEvent.GenericEventHandler += Raise.With(raisedWithArgs).Now;

// same as above, but without Now
TypeWithEvent.EventHandler += Raise.With(raisedWithArgs);
TypeWithEvent.GenericEventHandler += Raise.With(raisedWithArgs);

// for delegate void TimedEvent(DateTime eventStart):
TypeWithEvent.EventStarted += Raise.With<TimedEvent>(raisedWithTime);

// for event Action<int, string> ActionEvent;
TypeWithEvent.ActionEvent += Raise.With<Action<int, string>>(raisedWithInt, raisedWithString);

The implementation is rough, and is lacking in a number of areas:

1. thread safety of Raise.EventHandlerArguments
2. cleanup of "used" events, to avoid a resource-leak in the ever-growing Raise.EventHandlerArguments
3. argument type-checking (We get an exception if providing the wrong argument types to the delegate-style events, but it's not a nice one. I think an approach similar to ValueProducerSignatureHelper.AssertThatValueProducerSignatureSatisfiesCallSignature can be used to make a nice message.)

I will continue to work on those points, but wanted your feedback on the API (and of course welcome anything you have to say about the implementation).

Things that I anticipate comments on, and/or am bothered by:

1. I don't know how to get rid of the <TimedEvent> and <Action<int, string>> requirement; the compiler seems unable to figure out the required type. Do we consider this to be a problem? If so, any ideas how to deal with it?
2. Do you think reusing the Raise.With syntax introduces confusion?
3. the Raise.EventHandlerArguments implementation, being a static member on Raise, lacks finesse. I don't mind so much, if you prefer a proper class that's found using the service locator and whatnot, I can certainly oblige.
4. I didn't bother putting a Now on RaiseDelegate. We could. But why require the extra typing (I know, it provides consistency of interface, but people can have that by removing their existing Nows. It's kind of a gentle nudge…)

Fun facts, if we continue down this road:

• We could obsolete Now at any point. As far as I can tell, every event-raising works with our without it, in C# or VB.

Anyhow, I'm content to carry on, but thought maybe you'd like to put me on a better path if you see one.

(Also, sorry, @philippdolder, I seem to have taken over your project.)

from fakeiteasy.

I know I'm coming in late to this discussion, but something to consider (I apologize if I missed this in past discussion): .NET 4.5 no longer constrains EventHandler to using something that derives from EventArgs. TEventArgs can now be anything.

Of course compiling for .NET 4.5 may not make sense depending on the goals of the project. But I made a few alterations to the source to bump it to 4.5 and remove the constraint, and was able to get everything working as expected (other than Silverlight projects which I did not load).

from fakeiteasy.

@bondsbw, thanks for the idea. I think we'd be happy to discuss this, but I wonder if it wouldn't be a better idea to take it into another issue, since it's not directly related to supporting non-EventHandler signatures, and could have complications around compiling for 4.5. In any event, I don't think my "support anything" work will preclude us from doing this in the near future anyhow.
Hmm. All those arguments apply to splitting off the removal of Now, now that I think of it. Ah well.

from fakeiteasy.

I've spun off the Now-removal into #359.

from fakeiteasy.

In case it wasn't clear, if my fix for #359 (or a reworked version of it) is merged, I have the rest of the fix for this issue waiting in https://github.com/blairconrad/FakeItEasy/tree/30-a-naive-approach-to-eliminating-now.

from fakeiteasy.

Note to Blair.

Potential cleanup issues:

• remove IEventRaiserArguments
• check the rendered event-raising spec messages
• EventCall properties and constructor tidying

from fakeiteasy.

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.

from fakeiteasy.

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).

from fakeiteasy.

Agree.

from fakeiteasy.

This issue has been fixed in release 2.0.0.

from fakeiteasy.