-
Notifications
You must be signed in to change notification settings - Fork 181
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix event subscription/unsubscription with null handler #2034
Fix event subscription/unsubscription with null handler #2034
Conversation
[Scenario] | ||
public void SubscribeWithNullHandler(IEvents fake, Exception? exception) | ||
{ | ||
"Given a fake with an event" | ||
.x(() => fake = A.Fake<IEvents>()); | ||
|
||
"When subscribing to the event with a null handler" | ||
.x(() => exception = Record.Exception(() => fake.UnsubscribedEvent += null)); | ||
|
||
"Then it should not throw" | ||
.x(() => exception.Should().BeNull()); | ||
} | ||
|
||
[Scenario] | ||
public void UnsubscribeWithNullHandler(IEvents fake, Exception? exception) | ||
{ | ||
"Given a fake with an event" | ||
.x(() => fake = A.Fake<IEvents>()); | ||
|
||
"When unsubscribing from the event with a null handler" | ||
.x(() => exception = Record.Exception(() => fake.UnsubscribedEvent -= null)); | ||
|
||
"Then it should not throw" | ||
.x(() => exception.Should().BeNull()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love adding these specs to EventRaisingSpecs
, which is about raising events, not subscribing or unsubscribing. But we don't have specs for event subscription/unsubscription, and I also don't love adding a new spec file just for that.
Arguably, EventRaisingSpecs
covers not only event raising, but also subscription/unsubscription. So maybe we should just rename it to EventSpecs
or something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Renaming to EventSpecs
suits me. We can do here or later, as pleases you.
Thanks, @thomaslevesque. Your changes look sufficient to me. But they were bigger than I anticipated. I may be missing something, but blairconrad@c0091b1 also makes the specs pass with less disruption to the original code (and less mucking about with nullable hints). Thoughts? |
I like your approach! It's simpler, and doesn't require |
8866261
to
bc16732
Compare
I do. I'm very protective of my IP. Don't be silly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @thomaslevesque!
Thanks for the assist! |
Fixes #2033