Change SimpleNetwork::wait_for_packet to return Option<Event>
#1836
Conversation
phip1611
left a comment
There was a problem hiding this comment.
LGTM and sensible, thanks! Please keep the name wait_for_packet tho. Further, please add this to the CHANGELOG.md file.
|
Thanks for the review, I updated the PR with the requested changes. |
| #[must_use] | ||
| pub const fn wait_for_packet(&self) -> &Event { | ||
| unsafe { &*(ptr::from_ref(&self.0.wait_for_packet).cast::<Event>()) } | ||
| pub fn wait_for_packet(&self) -> Option<Event> { |
There was a problem hiding this comment.
I know you’ve streamlined this with other parts of the code, and that makes sense - thanks for that! However, I'm wondering whether this should instead return Result<Event, SomeError>. When I’m waiting for a packet, I wouldn’t expect this to return None under normal circumstances. Because of that, Id prefer using a Result`.
What do you think, @nicholasbishop ?
There was a problem hiding this comment.
This function is not waiting for a packet, it's returning an event that waits for a packet, if the underlying implementation does not support the event, the function might return None.
A reference to an `Event` is useless since every api takes an `&mut Event`, now we return the `Event` by value.
|
For the record: I've created #1839 as possible follow-up ticket |
A reference to an
Eventis useless since every api takes an&mut Event, now we return theEventby value.I also changed the name fromwait_for_packettowait_for_packet_eventto matchInput::wait_for_key_eventandPointer::wait_for_input_event.Both of these are breaking changes but it's necessary since it's not usable as it is now.