EventHandler

[Jaykul]

May I recommend using EventHandler to let this API work with more of the .net framework?

For what it's worth, my primary driver is the fact that PowerShell basically requires events to use EventHandler, but lots of other framework tools and APIs (like Rx) expect this too.

[Inumedia]

Neat, definitely quite the clean up. Unfortunately I'm out on the road and won't be available to fully review this for a good 2 weeks still. I'll try to find some time throughout the week.

Not to nit-pic, but I've always been concerned about upgrading the required dependency of Newtonsoft on our end, as I feel it would be felt by anyone else using this library and they'd have to accommodate for it (Having two version of Newtonsoft running in the same AppDomain seems... tricky?)

Would you happen to have any idea about how that would work?

[Inumedia]

Also, definitely a big concern of mine is the fact that this would be breaking changes for basically anyone that uses this library. I'm not opposed to it, but I'd probably have to break it off into a different major version with regards to NuGet versioning.

[Jaykul]

It's definitely a breaking change. I've been keeping it in a private fork for a long time, because I figured there just wasn't any point if only PowerShell needed it, but I ran across a reference to Rx having the same assumption that events are EventHandlers, so I figured I'd share it, since it seems like perhaps it's a best practice to do it that way even when it's superfluous :wink:

I should have split this into three pull requests, really (I still could, if you like).

First, the bulk of these changes:

1. Renaming MessageHistory to make it easier to tell the RTC responses from the messages.
2. Renaming NewMessage to make it less weird that other classes derive from it.
3. Add the optional username overload to SendMessage

Second, updating the dependencies can be used or ignored. It's immaterial. I think you're right that it would be basically impossible to use multiple versions in a single app. I don't have any idea what version most people are using.

Finally, the change to EventHandler could stay in a fork -- or at least, could stay in a fork until you were ready to bump your major version and change compatibility. Honestly, I did a search just now and realized that there are a few other events in SlackSocket which I didn't convert yet, which need to be converted if you're going to take a breaking change to switch from Action to EventHandler.

On that note, I can keep that in my fork if it's not worth it to you -- I need it so that I can use the library from PowerShell, but like you said, it's a breaking change to a core feature.

Of course, the other option is that since you've only got a few events (5?) I could probably think of a way to do it differently (e.g. make the methods which throw those events virtual, and override them in derived class that uses EventHandlers instead of Actions). It's probably the lazy course to break the API.

[Inumedia]

I'm not super worried about the dependencies, I've done routine updates in the past and haven't received any flack about it from anyone, so I'm assuming it's not major.

That being said, I think I would kind-of prefer the EventHandler refactor be in its own PR outside of the other changes. That way I can focus on one big thing at a time ;)

[Jaykul]

I'm going to just toss this one, and I think I'm going to also not rename the classes at all, because although I think it would make things clearer, it's not worth it as a breaking change for you.